This diff is nothing but dedentation -- it's empty under
`git diff -b`. These with-statements are only needed for
a pretty narrow scope of code, so make that clear in the
source.
There are two different things you need to patch in order to get error
emails (at `/emails`) in dev. Flip one of them in dev all the time,
and make the comment on the other a bit more explicit.
When I added this "Deployed code" feature to the error reporting,
I apparently hadn't worked out enough of how this code works to
realize that `notify_server_error` may be in a different process,
at a different time and potentially even on a different machine
from the actual error being reported.
Given that architecture, all the data about the error must be computed
in `AdminNotifyHandler`, before sending the report through the queue,
or else it risks being wrong. The job of `notify_server_error` and
friends is only to format the data and send it off. So, move the
implementation of this feature in order to do that.
(@showell added some "nocoverage" directives here for code that
is hard to test (exceptions being thrown, deployment files not
existing) and that was originally part of a file that didn't
require 100% coverage)
This helps prevent them from diverging and getting different sets of
features and fixes. As a bonus, the email path gets a nice tweak that
the Zulip path has had for years, since f7f2ec0ac, which makes the
emails clearer and less broken-looking when logging a message with no
stack trace.
This deduplicates a little bit of logic, and also has us always put
things into `report` the same way.
Empirically an exception in this codepath is very rare, so we won't
complicate the code by trying to salvage a lot of partial information
if it happens -- just log the traceback, and try to get a minimal
notification sent of the bare fact this happened.
This name hasn't been right since f7f2ec0ac back in 2013; this handler
sends the log record to a queue, whose consumer will not only maybe
send a Zulip message but definitely send an email. I found this
pretty confusing when I first worked on this logging code and was
looking for how exception emails got sent; so now that I see exactly
what's actually happening here, fix it.
This is just a basic Dropbox webhook integration. It just
notifies a user when something has changed, it does not
specify what changed. Doing so would require storing data,
as Dropbox API was created mainly for file managers, not
integrations like this.
Closes#5672
We have shifted to a generic queue to send all the emails. This queue
can retry in case of network issues; this makes sure that the emails are
always sent.
This commit just copies all the code from MissedMessageSendingWorker
class to a new EmailSendingWorker class. All the logic to send an email
through a queue was already there. This commit only makes the logic
generic. It does so by creating a special purpose queue called
'email_senders' to send any type of email. To make
MissedMessageSendingWorker still work we derive it from
EmailSendingWorker. All the tests that were testing
MissedMessageSendingWorker now run against EmailSendingWorker.
Such payloads are generated when a GitLab repository has merge
request approvals enabled and a project member approves a merge
request. Approving is not the same as merging.
This reverts commit 620b2cd6e.
Contributors setting up a new development environment were getting
errors like this:
```
++ dirname tools/do-destroy-rebuild-database
[...]
+ ./manage.py purge_queue --all
Traceback (most recent call last):
[...]
File "/home/zulipdev/zulip/zproject/legacy_urls.py", line 3, in <module>
import zerver.views.streams
File "/home/zulipdev/zulip/zerver/views/streams.py", line 187, in <module>
method_kwarg_pairs: List[FuncKwargPair]) -> HttpResponse:
File "/usr/lib/python3.5/typing.py", line 1025, in __getitem__
tvars = _type_vars(params)
[...]
File "/usr/lib/python3.5/typing.py", line 277, in _get_type_vars
for t in types:
TypeError: 'ellipsis' object is not iterable
```
The issue appears to be that we're using the `typing` module from the
3.5 stdlib, rather than the `typing=3.6.2` in our requirements files,
and that doesn't understand the `Callable[..., HttpResponse]` that
appears in the definition of `FuncKwargPair`.
Revert for now to get provision working again; at least one person
reports that reverting this sufficed. We'll need to do more testing
before putting this change back in.
The name `create_logger` suggests something much bigger than what this
function actually does -- the logger doesn't any more or less exist
after the function is called than before. Its one real function is to
send logs to a specific file.
So, pull out that logic to an appropriately-named function just for
it. We already use `logging.getLogger` in a number of places to
simply get a logger by name, and the old `create_logger` callsites can
do the same.
From the docs:
> This function does nothing if the root logger already has handlers
> configured for it.
Which we do if we've started up Django and configured settings, and in
particular allowed Django to process `settings.LOGGING`.
So, cut it out -- all it can do is confuse people about how logging
works.
If we ever actually used the `log_format` parameter, this would be
doubly confused, because only the first call would have any effect.
Because calls to `create_logger` generally run after settings are
configured, these would override what we have in `settings.LOGGING` --
which in particular defeated any attempt to set log levels in
`test_settings.py`. Move all of these settings to the same place in
`settings.py`, so they can be overridden in a uniform way.
This is already the loglevel we set on the root logger, so this has no
effect -- except in tests, where `test_settings.py` attempts to set
some of these same loggers to higher loglevels. Because the
`create_logger` call generally runs after we've configured settings,
it clobbers that effect.
The code in `test_settings.py` that tries to suppress logs only works
because it also sets `propagate=False`, which has nothing to do with
loglevels but does cause logs at this logger (and descendants) to be
dropped completely unless we've configured handlers for this logger
(or one of its relevant descendants.)
Adds a markdown preprocessor that finds ordered lists where all items
use the same number and change them to be in normal increasing order,
starting with that number.
Fixes#5159.
Eventually this check for the realm will be done in get_object_from_key
itself. Rewriting this to fit the pattern in get_object_from_key.
No change to behavior.
Commit d4ee3023 and its parent have the history behind this code.
Since d4ee3023^, all new PreregistrationUser objects, except those for
realm creation, have a non-None `realm`. Since d4ee3023, any legacy
PreregistrationUsers, with a `realm` of None despite not being for
realm creation, are treated as expired. Now, we ignore them
completely, and remove any that exist from the database.
The user-visible effect is to change the error message for
registration (or invitation) links created before d4ee3023^ to be
"link does not exist", rather than "link expired".
This change will at most affect users upgrading straight from 1.7 or
earlier to 1.8 (rather than from 1.7.1), but I think that's not much
of a concern (such installations are probably long-running
installations, without many live registration or invitation links).
[greg: tweaked commit message]
We should omit these for mypy. For most class definitions,
mypy doesn't need `Any`, and it provides no real useful info.
For clever monkeypatches, you should provide a more specific
type than `Any`.
The original logic is buggy now that emails can belong to (and be
invited to) multiple realms.
The new logic in the `invites` queue worker also avoids the bug where
when the PreregistrationUser was gone by the time the queue worker got
to the invite (e.g., because it'd been revoked), we threw an exception.
[greg: fix upgrade-compatibility logic; add test; explain
revoked-invite race above]
This code changes frequently enough that errors are bound to creep in. The
main change is that this sends the original invitation email instead of the
reminder email, but I think that's fine.
We'll need the expanded test coverage when we move
check_prereg_key_and_redirect to zerver/views/registration.py to avoid
test failures, and these are also tests we should really have anyway.
Empirically, the retry in `_on_connection_closed` didn't actually work
-- if a reconnect failed, that was it, and the exception handler
didn't get run. A traceback would get logged, but all its frames were
in Tornado or Pika, not our own code; presumably something magic and
async was happening to the exception.
Moreover, though we would make one attempt to reconnect if we had a
connection that got closed, we didn't have any form of retry if the
original attempt at connecting failed in the first place.
Happily, upstream offers a perfectly reasonable bit of API that avoids
both of these problems: the on-open-error callback. So use that.
This method was new in Tornado 4.0. It saves us from having to get
the time ourselves and do the arithmetic -- which not only makes the
code a bit shorter, but also easier to get right. Tornado docs (see
http://www.tornadoweb.org/en/stable/ioloop.html) say we should have
been getting the time from `ioloop.time()` rather than hardcoding
`time.time()`, because the loop could e.g. be running on the
`time.monotonic()` clock.
Adding it afterward is inherently racy, and upstream's API is quite
reasonable for avoiding that -- just like we can pass an on-open
callback up front, we can do the same with the on-close callback.
This is a more thorough version of 4adf2d5c2 from back in 2013-04.
The default value of this parameter is already False upstream.
(It was already False in pika version 0.9.6, which we were
supposedly using when we introduced this in 4baeaaa52; not sure
what the story was there.)
Previously, we weren't doing a proper left join in
user_groups_in_realm_serialized, resulting in empty user groups being
excluded from the query. We want to leave decisions about excluding
empty user groups to the UI layer, so we include these here.
These are the exceptions to the rule that our queues correspond to
queue-processor workers.
Purging `notify_tornado` in particular is a useful workaround right
now for some error spew in the dev environment.
Because we use access_stream_by_id here, and that checks for an active
subscription to interact with a private stream, this didn't work.
The correct fix to add an option to active_stream_by_id to accept an
argument indicating whether we need an active subscription; for this
use case, we definitely do not.
For example, this means that if a user already has an account on one
realm and they try to make an account on another by hitting "Sign in
with Google" (rather than following the little "Register" link to a
"Sign up with Google" button instead), they'll get to make an account
instead of getting an error.
Until very recently, if the user existed on another realm, any attempt
to register with that email address had to fail in the end, so this
logic gave the user a useful error message early. We introduced it in
c23aaa178 "GitHub: Show error on login page for wrong subdomain"
back in 2016-10 for that purpose. No longer! We now support reusing
an email on multiple realms, so we let the user proceed instead.
This function's interface is kind of confusing, but I believe when its
callers use it properly, `invalid_subdomain` should only ever be true
when `user_profile` is None -- in which case the revised
`invalid_subdomain` condition in this commit can never actually fire,
and the `invalid_subdomain` parameter no longer has any effect. (At
least some unit tests call this function improperly in that respect.)
I've kept this commit to a minimal change, but it would be a good
followup to go through the call sites, verify that, eliminate the use
of `invalid_subdomain`, then remove it from the function entirely.
[Modified by greg to (1) keep `USERNAME_FIELD = 'email'`,
(2) silence the corresponding system check, and (3) ban
reusing a system bot's email address, just like we do in
realm creation.]
As we migrate to allow reuse of the same email with multiple realms,
we need to replace the old "no email reuse" validators. Because
stealing the email for a system bot would be problematic, we still ban
doing so.
This commit only affects the realm creation logic, not registering an
account in an existing realm.
Just now this is largely redundant with `test_signup_already_active`;
but very soon when we allow reusing an email across realms, the logic
will diverge.
The one thing this bit of logic is used for is to decide whether
there's an existing user which is a mirror dummy that we should
activate. This change causes us to ignore such an existing user if
it's on some other realm, and go straight into `do_create_user`.
This code appears to exist to cover a few extra lines in
zerver/lib/digest.py. But it's rather brittle, tucked as it is into
the middle of a different test's loop, and with the upcoming
introduction of the `lear` realm in testing, this test code itself
loses coverage.
For now, rather than fix this test code up just delete it; we don't
have 100% coverage on `zerver/lib/digest.py`, while we do on this test
file, so that avoids breaking coverage in CI. As a followup, we
should add back some logic like this but in a more robust way,
probably as its own separate test method.
This test would produce a bunch of log messages with tracebacks,
complaining that `welcome-bot@zulip.com` tried to send cross-realm PMs
and can't. The issue is that the test overrides
`settings.CROSS_REALM_BOT_EMAILS`, and hasn't kept up with additions
to the normal value for that setting. Update it so that welcome-bot
is permitted as usual.
Originally this used signals, namely SIGRTMIN. But in prod, the
signal handler never fired; I debugged fruitlessly for a while, and
suspect uwsgi was foiling it in a mysterious way (which is kind of
the only way uwsgi does anything.)
So, we listen on a socket. Bit more code, and a bit trickier to
invoke, but it works.
This was developed for the investigation of memory-bloating on
chat.zulip.org that led to a331b4f64 "Optimize query_all_subs_by_stream()".
For usage instructions, see docstring.
In two factor authentication every step adds a unique prefix to the fields,
due to this the name of the form fields differs from the HTML fields. If
we do not do this we will have to change the name in the HTML, which
will cause the change in tests.
There's one migration required by this release:
* queue_processors: Stop passing state_handler to handle_message.
state_handler is now a property of bot_handler and thus, does
not need to be passed to bot_handler.handle_message().
The commit responsible is:
2a74ad11c5
This completes the last commit's work to fix CVE-2017-0910, applying
to any invite links already created before the fix was deployed. With
this change, all new-user registrations must match an explicit realm
in the PreregistrationUser row, except when creating a new realm.
[greg: rewrote commit message]
We would allow a user with a valid invitation for one realm to use it
on a different realm instead. On a server with multiple realms, an
authorized user of one realm could use this (by sending invites to
other email addresses they control) to create accounts on other
realms. (CVE-2017-0910)
With this commit, when sending an invitation, we record the inviting
user's realm on the PreregistrationUser row; and when registering a
user, we check that the PregistrationUser realm matches the realm the
user is trying to register on. This resolves CVE-2017-0910 for
newly-sent invitations; the next commit completes the fix.
[greg: rewrote commit message]
This fixes some subtle JavaScript exceptions we've been getting in
zulipchat.com, caused by the system bot realm there not being "zulip"
interacting with get_cross_realm_users.
This should help protect us from future issues with the way that
`bulk_get_users` does caching.
It's likely that we'll want to further restructure `bulk_get_users` to
not have this base_query code path altogether (since it's kinda
buggy), but I'm going to defer that for a time when we have another
user.
The previous implementation had a subtle caching bug: because it was
sharing its cache with the `get_user_profile_by_email` cache, if a
user happened to have an email in that cache, we'd return it, even
though that user didn't match `base_query`.
This causes `get_cross_realm_users` to no longer have a problematic
caching bug.
Hides URL if the message content == image url so that sending gifs or
images feels less cluttered. Uses the url_to_a() function to generate
the expected url string for matching.
Fixes#7324.
Appends "Test: " text to some tests to make changes to the image preview
rendering. In the future, if the message is only a link to an image,
the link will be hidden.
We include ERROR_BOT in this set, even though it's not technically
cross-realm (it just lives in the admin realm).
This code path does not correctly handle emails that correspond to
multiple accounts (because `get_system_bot` does not). Since it's
intended to only be used by system bots, we add an appropriate
assertion to ensure it is only used for system bots.
This was causing problems, because internal_send_message assumes that
there is a unique user (across all realms) with the given email
address (which is sorta required to support cross-realm bot messages
the way it does).
With this change, it now, in practice, only sends cross-realm bot
messages.
We now ignore payloads where payload['push']['changes'] is empty,
because an empty push doesn't really convey any useful information.
I couldn't find a way to replicate the action that would generate
such a payload, so I took one of our existing payloads and editted
out payload['push']['changes'] myself, so this payload is not
authentic.
Previously, this was a ValidationError, but that doesn't really make
sense, since this condition reflects an actual bug in the code.
Because this happened to be our only test coverage the ValidationError
catch on line 84 of registration.py, we add nocoverage there for now.
This buggy logic from e1686f427c had
broken do-destroy-rebuild-test-database.
Now that we're not just trying to add the Recipient objects for every
user on the system here to profiles_by_id, we also shouldn't be
processing every Recipeint object on the system. The fix is simple:
because of the patch we got merged into Django upstream,
recipients_to_create actually has the object IDs added to the
Recipient objects passed into Recipient.objects.bulk_create.
This was missed in manual testing, since it only broke `populate_db
--test-suite`.
An Integration object doesn't need access to the context dict used
to render its doc.md, since the context dict is just passed directly to
render_markdown_path.
Previously, when rendering a single integration, we tacked on the
following information to the context dict that was redundant:
* An OrderedDict containing all of the Integration objects for
all integrations.
* An OrderedDict containing all of the integration categories.
The context dict for rendering a particular integration doc would
contain 4 OrderedDicts, 2 for categories, 2 for Integration objects
because of how many times add_integrations_context had been called.
This was very wasteful, since an Integration object doesn't need
to access any other Integration object (or itself for that matter)
to render its documentation. This commit adds a function that
allows us to only pass in the context values that are necessary.
This is checked for in the caller of OurAuthenticationForm, which
meant this code was never run. But it is worth having an assertion
here to catch any possible regressions.
Structurally, the main change here is replacing the `clean_username`
function, which would get called when one accessed
self.cleaned_data['username'] with code in the main `clean` function.
This is important because only in `clean` do we have access to the
`realm` object.
Since I recently added full test coverage on this form, we know each
of the major cases have a test; the error messages are unchanged.
This deletes the old mock-covered test for this, which was mostly
useless. We have a much less messy test, which we extend to provide
the same test coverage the old one did.
While the result was the same before, this makes it more obvious.
This fixes a bug where, when a user is unsubscribed from a stream,
they might have unread messages on that stream leak. While it might
seem to be a minor problem, it can cause significant problems for
computing the `unread_msgs` data structures, since it means we need to
add an extra filter for whether the user is still subscribed, either
in the backend or in the UI.
Fixes#7095.
This code path was only required because we had remote_user set as a
positional argument here, and thus we'd be running this auth backend's
code when actually using another auth backend (due to how Django auth
backends are selected based on argument signature).
Inorder to provide more explicit error messages I have merged the
`emoji_code_is_valid()` and `emoji_name_is_valid()` functions into
`check_emoji_code_consistency()` and `check_emoji_name_consistency()`
respectively.
The installation admin is not the right person to get support requests from
deactivated users, regardless of the situation.
Also updates the wording to be a bit more concise.
This often can cause minor caching problems.
Obviously, it'd be better if we had access to the AST and thus could
do this rule for UserProfile objects in general.
This was basically rewritten by tabbott, because the code is a lot
cleaner after just rewriting the ZulipPasswordResetForm code to no
longer copy the model of the original Django version.
Fixes#4733.
Payloads that don't have a payload['object_attributes']['action']
attribute are generated when GitLab sends a test payload to verify
if the webhook was set up successfully. In this case, we should
send a message notifying that the webhook was configured
successfully.
Instead of populating the context dict with integration-specific
information in render_markdown_path, we now do that in
zerver.views.integrations.integration_doc instead.
Fixes#7401.
Tweaked by tabbott to use cast to handle the typing issues here.
This adds tests for a new more cases. Some were already covered
elsewhere in the codebase, but it feels best for LoginTest to fully
cover OurAuthenticationForm.
The character ">" now only starts a blockquote if the resulting
blockquote would be non-empty. Thus, by itself, ">" is now
interpreted literally by bugdown, fixing #687. The message
with contents consisting of ">>>" is now parsed as a doubly
(not triply) nested blockquote with contents ">". Properly
formed blockquotes have identical behavior as before, but now
bugdown can no longer produce empty blockquotes as output.
Fixes#2886, #687.
Storage limititations are only set on the value of
a config entry, since this is the only user-accessible
part of the schema. Keys are statically set by each
embedded bot.
We don't have our linter checking test files due to ultra-long strings
that are often present in test output that we verify. But it's worth
at least cleaning out all the ultra-long def lines.
This endpoint will allow us to add/delete emoji reactions whose emoji
got renamed during various emoji infra changes. This was also a
required change for realm emoji migration.
This commit was tweaked significantly by tabbott for greater clarity
(with no changes to the actual logic).
When the RabbitMQ server disappears, we log errors like these:
```
Traceback (most recent call last):
File "./zerver/lib/queue.py", line 114, in json_publish
self.publish(queue_name, ujson.dumps(body))
File "./zerver/lib/queue.py", line 108, in publish
self.ensure_queue(queue_name, do_publish)
File "./zerver/lib/queue.py", line 88, in ensure_queue
if not self.connection.is_open:
AttributeError: 'NoneType' object has no attribute 'is_open'
During handling of the above exception, another exception occurred:
[... traceback of connection failure inside the retried self.publish()]
```
That's a type error -- a programming error, not an exceptional
condition from outside the program. Fix the programming error.
Also move the retry out of the `except:` block, so that if it also
fails we don't get the exceptions stacked on each other. This is a
new feature of Python 3 which is sometimes indispensable for
debugging, and which surfaced this nit in the logs (on Python 2 we'd
never see the AttributeError part), but in some cases it can cause a
lot of spew if care isn't taken.
This commit helps reduce clutter on the navigation sidebar.
Creates new directories and moves relevant files into them.
Modifies index.rst, symlinks, and image paths accordingly.
This commit also enables expandable/collapsible navigation items,
renames files in docs/development and docs/production,
modifies /tools/test-documentation so that it overrides a theme setting,
Also updates links to other docs, file paths in the codebase that point
to developer documents, and files that should be excluded from lint tests.
Note that this commit does not update direct links to
zulip.readthedocs.io in the codebase; those will be resolved in an
upcoming follow-up commit (it'll be easier to verify all the links
once this is merged and ReadTheDocs is updated).
Fixes#5265.
While fixing an issue related to email gateway messages not getting
rendered properly, I unknowingly introduced a bug in the markdown
engine updation code. This commit fixes it. The issue was that for
a realm having email gateway setup, updation of realm filters would
lead to the updation of only one of the markdown engines not both.
In remove_members_from_group_backend, we are passing user group to
remove_members_from_user_group. In remove_members_from_user_group,
expect user_group_id.
This fixes a regression in ae5ba7f4fd,
where Zulip would 500 if the newly added system bots didn't exist on
the server.
This also fixes a moderate size performance problem where we'd fetch 5
users from memcached or the database in a loop.
This fixes a regression in 25c669df52.
We were draining the queue in both the superclass and the subclass,
so by the time the subclass started processing events, there were
no events to process. Now the subclass properly uses the events
passed in from the superclass.
These are new:
new-user-bot
emailgateway
Our cross-realm bots are hard coded to have email addresses
in the `zulip.com` domain, and they're not part of ordinary
realms.
These have always been cross-realm, but new enforcement in the
frontend code of all messages having been sent by a known user means
that it's important to add these properly.
This restyles and rewords some of the emoji style section to look
better and fit it more with the current style guide.
Tweaked by tabbott to modify the historical migration rather than
adding a new one. This is OK because the emojiset choices text change
doesn't touch the database; it's just a Django Python code thing.
Also removed translation tags, since we don't need them for a set of
brand names.
The intended use of $$ is for inline expressions, not for multiline
ones; ```math is an acceptable alternative for the latter. Hence,
the $$-syntax for inline TeX no longer permits newlines within it.
This was also necessary for the next change to be sensible; namely
allowing for spaces around both $$ when crafting inline TeX instead of
forcing everything to be crammed together, e.g. $$x=7$$. In order to
avoid uninentionally creating inline expressions, the opening and
closing $$'s of an inline expression must now both exactly consist of
two dollar signs, no more and no less.
Fixes: #6488.
The os.mkdir call is straightforward and doesn't testing.
Workers relying on LoopQueueProcessingWorker are tested
with its consume method that exists solely for this purpose.
Previously, these push notification events were being generated, but
then ignored in handle_push_notification because there was no
user_message object.
This should help make it easier to add tests coverage for these queue
processors, since they now use a system more similar to the other
queue processors.
In python3 base64.b64decode() can take an ASCII string, and any
legit data will be ASCII. If you pass in non-ASCII data, the
function will properly throw a ValueError (verified in python3 shell).
>>> s = '안녕하세요'
>>> import base64
>>> base64.b64decode(s)
Traceback (most recent call last):
File "/srv/zulip-py3-venv/lib/python3.4/base64.py", line 37, in _bytes_from_decode_data
return s.encode('ascii')
UnicodeEncodeError: 'ascii' codec can't encode characters in position 0-4: ordinal not in range(128)
During handling of the above exception, another exception occurred:
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
File "/srv/zulip-py3-venv/lib/python3.4/base64.py", line 83, in b64decode
s = _bytes_from_decode_data(s)
File "/srv/zulip-py3-venv/lib/python3.4/base64.py", line 39, in _bytes_from_decode_data
raise ValueError('string argument should contain only ASCII characters')
ValueError: string argument should contain only ASCII characters
Generally emails are not written with markdown in mind and hence
sometimes render in strange ways. This commit fixes a particular
issue that was causing whitespace before paragraphs to be treated
as code block due to which email content was being rendered in a
box that scrolls in right direction a lot.
Fixes: #7045.
In templates/zerver/api/main.html, since the current context isn't
passed to render_markdown_path when rendering an article,
render_markdown_path doesn't have the context to render values such
as api_url. This commit makes sure that it does by passing a dict
called api_uri_context to render_markdown_path when rendering an
article.
This commit puts the guts of parse_usermessage_flags into
UserMessage.flags_list_for_flags, since it was slightly faster
than the old implementation and produced the same results.
(Both algorithms were super fast, actually.)
And then all callers use the model method now.
The logic to set search_fields was essentially the same for both
sides of the include_history conditional.
Now we have just one code block that sets search_fields, and we
can quickly short-circuit the loop when is_search is False.
This change affects realm_users and realm_non_active_users.
Note that we still send full avatar urls in realm_user/add
events, so apply_events has to do something mildly hacky to
turn the avatar_url to None in that case.
Fixing the event is probably not worth the trouble, as single
urls are not bandwidth hogs; we only need this optimization
for bulk data.
This change affects these values:
* page_params.avatar_url
* page_params.avatar_url_medium
It requires passing the client_gravatar flag through this
codepath:
* home_real
* do_events_register
* fetch_initial_state_data
* avatar_url
Seems like the more logical check. Also, the previous code makes it feel
like there is a potential vulnerability where one could get an email change
object in a realm where email changes are disabled, and then open that link
while logged in to a different realm.
While we're at it, remove the unnecessary check that the user is
logged in when clicking the confirmation link; that creates
unnecessary trouble for users who use multiple browsers.
Removes an assert, which at this point is there just for readability, since
the second argument to
get_object_from_key(confirmation_key, Confirmation.EMAIL_CHANGE)
ensures that the returned object is of the correct type.
This commit allows clients to register client_gravatar=True, and
then we recognize that flag for message events. If the flag is
True, we will not calculate gravatar URLs and let the clients do
it themselves. (Clients can calculate gravatar URLs based on
emails with just a little bit of code.)
This refactoring doesn't change behavior, but it sets us up
to more easily handle a register setting for `client_gravatar`,
which will allow clients to tell us they're going to compute
their own gravatar URLs.
The `client_gravatar` flag already exists in our code, but it
is only used for Django views (users/messages) but not for
Zulip events.
The main change is to move the call to `set_sender_avatar` into
`finalize_payload`, which adds the boolean `client_gravatar`
parameter to that function. And then we update various callers
to supply that flag.
One small performance benefit of this change is that we now
lazily compute the client message payloads in
`event_queue.process_message_event` now, so this will improve
performance if all interested clients have the same value of
`apply_markdown`. But the change here is really preparing us
for the additional boolean parameter, which will cause us to
have four variations of the payload.
This gets used when we call `process_client`, which we generally do at
some kind of login; and in particular, we do in the shared auth
codepath `login_or_register_remote_user`. Add a decorator to make it
easy, and use it on the various views that wind up there.
In particular, this ensures that the `query` is some reasonable
constant corresponding to the view, as intended. When not set, we
fall back in `update_user_activity` on the URL path, but in particular
for `log_into_subdomain` that can now contain a bunch of
request-specific data, which makes it (a) not aggregate properly, and
(b) not even fit in the `CHARACTER VARYING(50)` database field we've
allotted it.
The only place this attribute is used is in `update_user_activity`,
called only in `process_client`, which won't happen if we end up
returning a redirect just below. If we don't, we go and call
`add_logging_data` just after, which takes care of this already.
This won't work for all call paths without deeper refactoring,
but for at least some paths we can make this more direct -- function
arguments, rather than mutating a request attribute -- so it's easier
to see how the data is flowing.
I remember being really confused by this function in the past, and I finally
figured it out. It should be removed, and the dev_url added by
00-realm-creation should call a function that just gets the confirmation_key
from outbox like all of the backend tests, but until then this comment
should help.
This change:
* Prevents weird potential attacks like taking a valid confirmation link
(say an unsubscribe link), and putting it into the URL of a multiuse
invite link. I don't know of any such attacks one could do right now, but
reasoning about it is complicated.
* Makes the code easier to read, and in the case of confirmation/views.py,
exposes something that needed refactoring anyway (USER_REGISTRATION and
INVITATION should have different endpoints, and both of those endpoints
should be in zerver/views/registration, not this file).
This test helper method duplicated a bunch of logic in
`zerver/worker/queue_processors.py` in a specialized fashion for the
tests. Now that we're using `call_consume_in_tests` in this code
path, we don't need it.
Before this commit, ResponseMock() was initialized
with a data attribute, which isn't used in the tests
and does not occur in the outgoing webhook code.
The main limitation of this version is that it's controlled entirely
from settings, with nothing in the database and no web UI or even
management command to control it. That makes it a bit more of a
burden for the server admins than it'd ideally be, but that's fine
for now.
Relatedly, the web flow for realm creation still requires choosing a
subdomain even if the realm is destined to live at an alias domain.
Specific to the dev environment, there is an annoying quirk: the
special dev login flow doesn't work on a REALM_HOSTS realm. Also,
in this version the `add_new_realm` and `add_new_user` management
commands, which are intended for use in development environments only,
don't support this feature.
In manual testing, I've confirmed that a REALM_HOSTS realm works for
signup and login, with email/password, Google SSO, or GitHub SSO.
Most of that was in dev; I used zulipstaging.com to also test
* logging in with email and password;
* logging in with Google SSO... far enough to correctly determine
that my email address is associated with some other realm.
The original PR to allow generic bots to be mentioned had
some merge issues that we detected about a week after the
fact. This commit restores the logic from the original PR.
The reason we didn't detect this bug earlier is that the
merge issues didn't break any existing behavior. Instead,
they made it so that only UserMessage rows got written for
bots, but no events were being set. The part of the commit
that got lost is restored here, so now events get sent as
well.
Thanks to @derAnfaenger for reporting this and being patient
as we tracked it down.
Fixes#7140
This adds the data model and bugdown support for the new UserGroup
mention feature.
Before it'll be fully operational, we'll still need:
* A backend API for making these.
* A UI for interacting with that API.
* Typeahead on the frontend.
* CSS to make them look pretty and see who's in them.
tsearch_extras returns search offsets in bytes but our highlight
function treated them as character offsets. Added a check to subtract
extra bytes if the tsearch search backend is being used.
Fixes#4084.
Fixes#7021.
This commit allows for the /api-new/ page to rendered similarly to our
/help pages. It's based on the old content for /api, but we're not
replacing the old content yet, to give a bit of time to restructure
things reasonably.
Tweaked by eeshangarg and tabbott.
Because this is for tests, a heuristic like this that's right in most
situations is actually fine; we can override it in the few cases where
a test might set up a situation where it fails.
So just make it clear for the next reader that that's what's going on,
and also adjust the helper's interface slightly so that its callers
do have that flexibility.
The "subdomain" label is redundant, to the extent it's even
accurate -- this is really just the URL we want to display,
which may or may not involve a subdomain. Similarly "external".
The former `external_api_path_subdomain` was never a path -- it's a
host, followed by a path, which together form a scheme-relative URL.
I'm not quite convinced that value is actually the right thing in
2 of the 3 places we use it, but fixing that can start by giving an
accurate name to the thing we have.
This was part of the logic to handle EXTERNAL_API_PATH varying.
But also it was already no longer used -- it was only ever passed
into template contexts, as `external_api_uri`, and it'd been
overtaken there by `external_api_uri_subdomain`.
So, update our dev docs to reflect that, and eliminate the variable.
This setting isn't documented at all, and I believe nobody has used it
since the end of api.zulip.com in 2016. So we get to complete the
cleanup of this logic.
We extract get_bulk_stream_subscriber_info() from this
function to remove some of the complexity. Also, in that
new function we avoid a hop to the database by querying
on stream ids instead of recipient ids. The query that
gets changed here does require a join to the recipient
table (to get the stream id), so it's a little bit of a
tradeoff.
There's an implicit assumption in bulk_remove_subscriptions
that all users belong to the same realm. We use the realm
for things like comparing occupied streams before and
after our main operation of deactivating streams.
Before this change, we just used the user_profile variable
that leaked from some prior loop to look up the realm, which
was super brittle.
Now we're a bit more explicit.
We were using Google's diff-match-patch library to diff HTML. The
problem with that approach is that it is a text differ, not an HTML
differ and so it ends up messing up the HTML tags. `lxml` is a safer
option.
Fixes: #7219.
Note that this code leads to a slightly different query, because
we join to one row in the small Recipient table to match
stream_id to recipient.type_id.
The first method we extract to this library is
get_active_subscriptions_for_stream_id().
We also move num_subscribers_for_stream_id() to here, which
is slightly annoying (having the method on Stream was nice)
but avoids some circular dependency issues.
The HelpView class will render a directory as markdown with an index HTML
page. This however can also be used for other generics and applied to
the API pages as well, so change the class to a generic class and
specify the path templates and names.
Tweaked by tabbott and Eeshan Garg.
FuncT was unused in decorator.py, and only imported into profile.py.
The @profiled decorator is now more strongly typed on return-type.
Annotations were converted to python3 format.
This extraction moves all the huddle logic into models.py, which
hopefully can reduce friction for things like re-organizing our
caches (there are two cache entries for every huddle) and/or
just putting huddle_id on Message directly.
Do you call get_recipient(Recipient.STREAM, stream_id) or
get_recipient(stream_id, Recipient.STREAM)? I could never
remember, and it was not very type safe, since both parameters
are integers.
Almost all callers to do_create_user were trying to
create active users, except for one test. The
active=False codepath was kind of broken (things
like sending welcome messages had sort of undefined
behavior there), so instead of trying to maintain it,
we just update the one test (`test_people`) to flip the
`is_active` flag manually.
Fixes#7197
greysome
Xavier Cooney
Rishi Gupta
Steve Howell
Greg Price
neiljp (Neil Pilgrim)
Aastha Gupta
Angelika Serwa
Marco Burstein
Tommy Ip
Umair Khan
Vishnu Ks
snlkapil
ViRu-ThE-ViRuS
Eeshan Garg
Andy Perez
Andy Perez
Callum Fraser
civilnewsnetwork
Alena Volkova
Sivagiri
rht
Weronika Grzybowska
Rhea Parekh
kunall17
Shreyansh Dwivedi
Ivche1337
Arseny Chekanov
Rohitt Vashishtha
Tim Abbott
Sarah
derAnfaenger
Sampriti Panda
Vaibhav Sagar
Harshit Bansal
Shubham Dhama
Greg Price
David Rosa Tamsen
Brock Whittaker
YJDave
Aditya Bansal
Felix Yan