`zulip.zerver.lib.webhooks.common` was very opaque previously,
especially since none of the logging was actually done from that
module.
Adjust to a more explicit logger name.
Any exception is an "unexpected event", which means talking about
having an "unexpected event logger" or "unexpected event exception" is
confusing. As the error message in `exceptions.py` already explains,
this is about an _unsupported_ event type.
This also switches the path that these exceptions are written to,
accordingly.
For web-public streams, clients can access full topic history
without being authenticated. They only need to additionally
send "streams:web-public" narrow with their request like all
the other web-public queries.
By default, the Django Sentry integration provides the email address
and username of pulled from the auth layer. This is potentially PII,
and not data that we wish to store. Enable sending user data at all,
by setting `send_default_pii=True`, but strip the username and
email (which are the same, in Zulip) before sending. Users will be
identified in Sentry only by their IP address, user ID, realm, and
role.
The return type of `ugettext_lazy('...')` (aliased as `_`) is a
promise, which is only forced into a string when it is dealt with in
string context. This `django.utils.functional.lazy.__proxy__` object
is not entirely transparent, however -- it cannot be serialized by
`orjson`, and `isinstance(x, str) == False`, which can lead to
surprising action-at-a-distance.
In the two places which will serialize the role value (either into
Zulip's own error reporting queue, or Sentry's), force the return
value. Failure to do this results in errors being dropped
mostly-silently, as they cannot be serialized and enqueued by the
error reporter logger, which has no recourse but to just log a
warning; see previous commit.
When we do this forcing, explicitly override the language to be the
realm default. Failure to provide this override would translate the
role into the role in the language of the _request_, yielding varying
results.
596cf2580b ignored the loggers of all SuspiciousOperation subclasses,
but not SuspiciousOperation itself. Almost all locations raise one of
the more specific subclasses, with the exception of one location in
the session middleware[1].
Ignore the overall django.security.SuspiciousOperation logger as well.
[1] https://code.djangoproject.com/ticket/31962
This commit adds automatic detection of extra output (other than
printed by testing library or tools) in stderr and stdout by code under
test test-backend when it is run with flag --ban-console-output.
It also prints the test that produced the extra console output.
Fixes: #1587.
Extracting a section for presence endpoints and using path() rather
than re_path() results in a much cleaner implementation of this
concept.
This eliminates the last case where test_openapi couldn't correctly
match an endpoint documentation with the OpenAPI definitions for it.
Via API, users can now access messages which are in web-public
streams without any authentication.
If the user is not authenticated, we assume it is a web-public
query and add `streams:web-public` narrow if not already present
to the narrow. web-public streams are also directly accessible.
Any malformed narrow which is not allowed in a web-public query
results in a 400 or 401. See test_message_fetch for the allowed
queries.
django.security.DisallowedHost is only one of a set of exceptions that
are "SuspiciousOperation" exceptions; all return a 400 to the user
when they bubble up[1]; all of them are uninteresting to Sentry.
While they may, in bulk, show a mis-configuration of some sort of the
application, such a failure should be detected via the increase in
400's, not via these, which are uninteresting individually.
While all of these are subclasses of SuspiciousOperation, we enumerate
them explicitly for a number of reasons:
- There is no one logger we can ignore that captures all of them.
Each of the errors uses its own logger, and django does not supply
a `django.security` logger that all of them feed into.
- Nor can we catch this by examining the exception object. The
SuspiciousOperation exception is raised too early in the stack for
us to catch the exception by way of middleware and check
`isinstance`. But at the Sentry level, in `add_context`, it is no
longer an exception but a log entry, and as such we have no
`isinstance` that can be applied; we only know the logger name.
- Finally, there is the semantic argument that while we have decided
to ignore this set of security warnings, we _may_ wish to log new
ones that may be added at some point in the future. It is better
to opt into those ignores than to blanket ignore all messages from
the security logger.
This moves the DisallowedHost `ignore_logger` to be adjacent to its
kin, and not on the middleware that may trigger it. Consistency is
more important than locality in this case.
Of these, the DisallowedHost logger if left as the only one that is
explicitly ignored in the LOGGING configuration in
`computed_settings.py`; it is by far the most frequent, and the least
likely to be malicious or impactful (unlike, say, RequestDataTooBig).
[1] https://docs.djangoproject.com/en/3.0/ref/exceptions/#suspiciousoperation
There are three exceptions in Python3 which are descended from
BaseException, but not Exception: GeneratorExit, KeyboardInterrupt,
and SystemExit. None of these are suitable to be sent to Sentry.
For example, SystemExit is raised by `sys.exit`; in that sense, it is
never "uncaught" because we chose to cause it explicitly.
Use the suggested form[1] for ignoring specific classes of exceptions.
[1] https://github.com/getsentry/sentry-python/issues/149#issuecomment-434448781
Our intent throughout the codebase is to treat email
case-insensitively.
The only codepath affected by this bug is remote_user_sso, as that's the
only one that currently passes potentially both a user_profile and
ExternalAuthDataDict when creating the ExternalAuthResult. That's why we
add a test specifically for that codepath.
This commit adds EMAIL_PORT setting for explicitly specifying the
port of SMTP provider in dev_settings.py.
We also change email_backends.send_email_smtp to pass EMAIL_PORT
along with EMAIL_HOST to smtplib.SMTP.
After this change, we will not need to include the port along with
host in EMAIL_HOST.
Also updated the email.md docs accordingly for this change.
As part of issue #15344, the error report emails add the user role
information. This commit adds the user role information to be used
by sentry as well.
The apple developer webapp consistently refers this App ID. So,
this clears any confusion that can occur.
Since python social auth only requires us to include App ID in
_AUDIENCE(a list), we do that in computed settings making it easier for
server admin and we make it much clear by having it set to
APP_ID instead of BUNDLE_ID.
Uses git release as this version 3.4.0 is not released to pypi.
This is required for removing some overriden functions of
apple auth backend class AppleAuthBackend.
With the update we also make following changes:
* Fix full name being populated as "None None".
c5c74f27dd that's included in update assigns first_name and last_name
to None when no name is provided by apple. Due to this our
code is filling return_data['full_name'] to 'None None'.
This commit fixes it by making first and last name strings empty.
* Remove decode_id_token override.
Python social auth merged the PR we sent including the changes
we made to decode_id_token function. So, now there is no
necessity for the override.
* Add _AUDIENCE setting in computed_settings.py.
`decode_id_token` is dependent on this setting.
This lets us test the recursion bug behavior of this logging handler
without resulting in `logging.error` output being printed to the
console in the event that the test passes.
Use the default configuration, which catches Error logging and
exceptions. This is placed in `computed_settings.py` to match the
suggested configuration from Sentry[1], which places it in `settings.py`
to ensure it is consistently loaded early enough.
It is placed behind a check for SENTRY_DSN soas to not incur the
additional overhead of importing the `sentry_sdk` modules if Sentry is
not configured.
[1] https://docs.sentry.io/platforms/python/django/
Fixes#15904.
settings is supposed to be a proper OneLogin_Saml2_Settings object,
rather than an empty dictionary. This bug wasn't easy to spot because
the codepath that causes this to demonstrate runs only if the
SAMLResponse contains encrypted assertions.
In particular importing gitter data leads to having accounts with these
noreply github emails. We generally only want users to have emails that
we can actually send messages to, so we'll keep the old behavior of
disallowing sign up with such an email address. However, if an account
of this type already exists, we should allow the user to have access to
it.
A few major themes here:
- We remove short_name from UserProfile
and add the appropriate migration.
- We remove short_name from various
cache-related lists of fields.
- We allow import tools to continue to
write short_name to their export files,
and then we simply ignore the field
at import time.
- We change functions like do_create_user,
create_user_profile, etc.
- We keep short_name in the /json/bots
API. (It actually gets turned into
an email.)
- We don't modify our LDAP code much
here.
This particular commit has been a long time coming. For reference,
!avatar(email) was an undocumented syntax that simply rendered an
inline 50px avatar for a user in a message, essentially allowing
you to create a user pill like:
`!avatar(alice@example.com) Alice: hey!`
---
Reimplementation
If we decide to reimplement this or a similar feature in the future,
we could use something like `<avatar:userid>` syntax which is more
in line with creating links in markdown. Even then, it would not be
a good idea to add this instead of supporting inline images directly.
Since any usecases of such a syntax are in automation, we do not need
to make it userfriendly and something like the following is a better
implementation that doesn't need a custom syntax:
` Alice: hey!`
---
History
We initially added this syntax back in 2012 and it was 'deprecated'
from the get go. Here's what the original commit had to say about
the new syntax:
> We'll use this internally for the commit bot. We might eventually
> disable it for external users.
We eventually did start using this for our github integrations in 2013
but since then, those integrations have been neglected in favor of
our GitHub webhooks which do not use this syntax.
When we copied `!gravatar` to add the `!avatar` syntax, we also noted
that we want to deprecate the `!gravatar` syntax entirely - in 2013!
Since then, we haven't advertised either of these syntaxes anywhere
in our docs, and the only two places where this syntax remains is
our game bots that could easily do without these, and the git commit
integration that we have deprecated anyway.
We do not have any evidence of someone asking about this syntax on
chat.zulip.org when developing an integration and rightfully so- only
the people who work on Zulip (and specifically, markdown) are likely
to stumble upon it and try it out.
This is also the only peice of code due to which we had to look up
emails -> userid mapping in our backend markdown. By removing this,
we entirely remove the backend markdown's dependency on user emails
to render messages.
---
Relevant commits:
- Oct 2012, Initial commit c31462c278
- Nov 2013, Update commit bot 968c393826
- Nov 2013, Add avatar syntax 761c0a0266
- Sep 2017, Avoid email use c3032a7fe8
- Apr 2019, Remove from webhook 674fcfcce1
We use the EMAIL_TIMEOUT django setting to timeout after 15s of trying
to send an email. This will nicely lead to retries in the email_senders
queue, due to the retry_send_email_failures decorator.
smtlib documentation suggests that socket.timeout can be raised as the
result of timing out, so in attempts I'm getting
smtplib.SMTPServerDisconnected. Either way, seems appropriate to add
socket.timeout to the exception that we catch.
I checked that this does not interfere with the MRO of the auth
backends:
In [1]: import zproject.backends; zproject.backends.GitHubAuthBackend.__mro__
Out[1]:
(zproject.backends.GitHubAuthBackend,
zproject.backends.SocialAuthMixin,
zproject.backends.ZulipAuthMixin,
zproject.backends.ExternalAuthMethod,
abc.ABC,
social_core.backends.github.GithubOAuth2,
social_core.backends.oauth.BaseOAuth2,
social_core.backends.oauth.OAuthAuth,
social_core.backends.base.BaseAuth,
object)
Signed-off-by: Anders Kaseorg <anders@zulip.com>
We remove support for the old clients which required an event for
each message to clear notification.
This is justified since it has been around 1.5 years since we started
supporting the bulk operation (and so essentially nobody is using a
mobile app version so old that it doesn't support the batched
approach) and the unbatched approach has a maintenance and reliability
cost.
There is still some miscellaneous cleanup that
has to happen for things like analytics queries
and dead code in node tests, but this should
remove the main use of pointers in the backend.
(We will also still need to drop the DB field.)
Due to authentication restrictions, a deployment may need to direct
traffic for mobile applications to an alternate uri to take advantage
alternate authentication mechansism. By default the standard realm URI
will be usedm but if overridden in the settings file, an alternate uri
can be substituted.
Because of other validation on these values, I don't believe any of
these does anything different, but these changes improve readability
and likely make GitHub's code scanners happy.
I believe the Bundle ID (aka App ID) and Services ID have meaning only
relative to a specific Team ID. In particular, in some places in the
developer.apple.com UI, they're displayed in a fully-qualified form
like "ABCDE12345.com.example.app", where "com.example.app" is the
App ID or Services ID and ABCDE12345 is the Team ID.
Adds the ability to set a SAML attribute which contains a
list of subdomains the user is allowed to access. This allows a Zulip
server with multiple organizations to filter using SAML attributes
which organization each user can access.
Cleaned up and adapted by Mateusz Mandera to fit our conventions and
needs more.
Co-authored-by: Mateusz Mandera <mateusz.mandera@zulip.com>
This migrations use of url() to path() or re_path(). In this commit,
we only migration regular expressions to path where the translation is trivial:
* URLs with no parameters in them
* URLs with only integer parameters in them
* Strings where there regular expression just checked for `/`s
path; strings, which can have variable validation in the URLs that
need by-hand auditing, we leave for future commits that are easier to
review and think about the individual changes.
Modified by tabbott to convert back to `re_path` various URLs with
strings that had been converted to use `path()` with string
validation to simplify review.
Fixes#14770.
Fixes#14960.
The default of 6 thread may not be appropriate in certain
configurations. Taking half of the numer of CPUs available to the
process will be more flexible.
Old: a validator returns None on success and returns an error string
on error.
New: a validator returns the validated value on success and raises
ValidationError on error.
This allows mypy to catch mismatches between the annotated type of a
REQ parameter and the type that the validator actually validates.
Signed-off-by: Anders Kaseorg <anders@zulip.com>
This prevents memcached from automatically appending the hostname to
the username, which was a source of problems on servers where the
hostname was changed.
Signed-off-by: Anders Kaseorg <anders@zulip.com>
The zerver.models hack does not appear to be necessary now.
Meanwhile, get_wsgi_application has its own django.setup call, which
would overwrite the parts of our logging configuration pulled in by
zerver.models.
This fixes part of #15391; specifically, fixes it in production, but
not in development, where ‘manage.py runserver’ calls its own
django.setup and then imports various bits of our code before finding
zproject.wsgi.
Signed-off-by: Anders Kaseorg <anders@zulip.com>
Overrides some of internal functions of python-social-auth
to handle native flow.
Credits to Mateusz Mandera for the overridden functions.
Co-authored-by: Mateusz Mandera <mateusz.mandera@zulip.com>
Fixes#2665.
Regenerated by tabbott with `lint --fix` after a rebase and change in
parameters.
Note from tabbott: In a few cases, this converts technical debt in the
form of unsorted imports into different technical debt in the form of
our largest files having very long, ugly import sequences at the
start. I expect this change will increase pressure for us to split
those files, which isn't a bad thing.
Signed-off-by: Anders Kaseorg <anders@zulip.com>
Automatically generated by the following script, based on the output
of lint with flake8-comma:
import re
import sys
last_filename = None
last_row = None
lines = []
for msg in sys.stdin:
m = re.match(
r"\x1b\[35mflake8 \|\x1b\[0m \x1b\[1;31m(.+):(\d+):(\d+): (\w+)", msg
)
if m:
filename, row_str, col_str, err = m.groups()
row, col = int(row_str), int(col_str)
if filename == last_filename:
assert last_row != row
else:
if last_filename is not None:
with open(last_filename, "w") as f:
f.writelines(lines)
with open(filename) as f:
lines = f.readlines()
last_filename = filename
last_row = row
line = lines[row - 1]
if err in ["C812", "C815"]:
lines[row - 1] = line[: col - 1] + "," + line[col - 1 :]
elif err in ["C819"]:
assert line[col - 2] == ","
lines[row - 1] = line[: col - 2] + line[col - 1 :].lstrip(" ")
if last_filename is not None:
with open(last_filename, "w") as f:
f.writelines(lines)
Signed-off-by: Anders Kaseorg <anders@zulipchat.com>
This new endpoint returns a 'user' dictionary which, as of now,
contains a single key 'is_subscribed' with a boolean value that
represents whether the user with the given 'user_id' is subscribed
to the stream with the given 'stream_id'.
Fixes#14966.
Fixes this error in the dev environment:
$ ./manage.py checkconfig
Error: You must set ZULIP_ADMINISTRATOR in /etc/zulip/settings.py.
Signed-off-by: Anders Kaseorg <anders@zulip.com>
The logger defines a full list of handlers, meaning propagate=False is
needed, to avoid the log line propagating further up the logging tree
and getting logged multiple times by the duplicated handlers.
This implementation overrides some of PSA's internal backend
functions to handle `state` value with redis as the standard
way doesn't work because of apple sending required details
in the form of POST request.
Includes a mixin test class that'll be useful for testing
Native auth flow.
Thanks to Mateusz Mandera for the idea of using redis and
other important work on this.
Documentation rewritten by tabbott.
Co-authored-by: Mateusz Mandera <mateusz.mandera@zulip.com>
This was written by Rishi for a very brief purpose a few years ago,
and it doesn't serve much purpose now other than to be a place we
update in code sweeps.
We're migrating to using the cleaner zulip.com domain, which involves
changing all of our links from ReadTheDocs and other places to point
to the cleaner URL.
This commit adds `name` attribute for the backends that do not
have them.
This is just a kind of prep commit in case if we want to use
`self.logger.xxxx()` in the future which is dependent on the
`name` attribute. But right now these logging calls aren't used
anywhere in those backends.
`HTTPError` has empty string for `str(HTTPError())`. Logging it
as it is would not be much helpful. So, this commits adds code
to log the name of error also.
Adds a top-level logger in `settings.LOGGING` `zulip.auth`
with the default handlers `DEFAULT_ZULIP_HANDLERS` and
an extra hanlder that writes to `/var/log/zulip/auth.log`.
Each auth backend uses it's own logger, `self.logger` which
is in form 'zulip.auth.<backend name>'.
This way it's clear which auth backend generated the log
and is easier to look for all authentication logs in one file.
Besides the above mentioned changes, `name` attribute is added to
`ZulipAuthMixin` so that these logging kind of calls wouldn't raise
any issues when logging is tried in a class without `name` attribute.
Also in the tests we use a new way to check if logger calls are made
i.e. we use `assertLogs` to test if something is logged.
Thanks to Mateusz Mandera for the idea of having a seperate logger
for auth backends and suggestion of using `assertLogs`.
Calling jwt.decode without an algorithms list raises a
DeprecationWarning. This is for protecting against
symmetric/asymmetric key confusion attacks.
This is a backwards-incompatible configuration change.
Fixes#15207.
Signed-off-by: Anders Kaseorg <anders@zulip.com>
Generated by pyupgrade --py36-plus --keep-percent-format, but with the
NamedTuple changes reverted (see commit
ba7906a3c6, #15132).
Signed-off-by: Anders Kaseorg <anders@zulip.com>
This reimplements our Zoom video call integration to use an OAuth
application. In addition to providing a cleaner setup experience,
especially on zulipchat.com where the server administrators can have
done the app registration already, it also fixes the limitation of the
previous integration that it could only have one call active at a time
when set up with typical Zoom API keys.
Fixes#11672.
Co-authored-by: Marco Burstein <marco@marco.how>
Co-authored-by: Tim Abbott <tabbott@zulipchat.com>
Signed-off-by: Anders Kaseorg <andersk@mit.edu>
We change do_create_user and create_user to accept
role as a parameter instead of 'is_realm_admin' and 'is_guest'.
These changes are done to minimize data conversions between
role and boolean fields.
Earlier this `standard_relay_params` was used only for SAML auth,
now "Sign in with Apple" also requires this to store those params
in session for reuse. So, this acts as a prep commit for "Sign in
with Apple" auth support.
This will protect us in case of some kinds of bugs that could allow
making requests such as password authentication attempts to tornado.
Without restricting the domains to which the in-memory backend can
be applied, such bugs would lead to attackers having multiple times
larger rate limits for these sensitive requests.
Helps to see if users are often trying to login with deactived
accounts.
A use case: Trackdown whether any deactivated bot users are still
trying to access the API.
This implementation adds a new key `inactive_user_id`
to `return_data` in the function `is_user_active` which
check if a `user_profile` is active. This reduces the effort
of getting `user_id` just before logging.
Modified tests for line coverage.
Instead of plumbing the idp to /complete/saml/ through redis, it's much
more natural to just figure it out from the SAMLResponse, because the
information is there.
This is also a preparatory step for adding IdP-initiated sign in, for
which it is important for /complete/saml/ to be able to figure out which
IdP the request is coming from.
If the IdP authentication API is flaky for some reason, it can return
bad http responses, which will raise HTTPError inside
python-social-auth. We don't want to generate a traceback
in those cases, but simply log the exception and fail gracefully.
While this functionality to post slow queries to a Zulip stream was
very useful in the early days of Zulip, when there were only a few
hundred accounts, it's long since been useless since (1) the total
request volume on larger Zulip servers run by Zulip developers, and
(2) other server operators don't want real-time notifications of slow
backend queries. The right structure for this is just a log file.
We get rid of the queue and replace it with a "zulip.slow_queries"
logger, which will still log to /var/log/zulip/slow_queries.log for
ease of access to this information and propagate to the other logging
handlers. Reducing the amount of queues is good for lowering zulip's
memory footprint and restart performance, since we run at least one
dedicated queue worker process for each one in most configurations.
It appears that a recent pika release started logging spammy INFO
output on the pika.connection and pika.channel channels, in addition
to the existing pika.adapters channel.
It's probably best to just move to WARNING-level logging for all of these.
This significantly cleans up the output when run-dev.py restarts
services due to a code change in the development environment.
Instead of having to filter `@noreply.github.com` emails in
`get_unverified_emails`, it's good to make `filter_usable_emails`
just filter `@noreply.github.com` and handle verified/unverified
part in their respective functions because of `@noreply.github.com`
exception being a fiddly special-case detail.
Also renamed `filter_usable_emails` to `get_usable_email_objects`
as a line that gets all associated github emails is removed in
`get_verified_emails` and `get_unverified_emails` and added to
`filter_usable_emails`. The name `filter_usable_emails` suggests
that it just filters given emails, whereas here it's getting all
associated email objects and returning usable emails.
This commit extends the template for "choose email" to mention for
users who have unverified emails that they need to verify them before
using them for Zulip authentication.
Also modified `social_auth_test_finish` to assert if all emails
are present in "choose email" screen as we need unverified emails
to be shown to user and verified emails to login/signup.
Fixes#12638 as this was the last task for that issue.
This separates the part of code that gets all the emails associated
to GitHub from `get_verified_emails` in `GitHubAuthBackend`.
Improves readability of code and acts as a preparatory commit for
extending the template for "choose email" in GitHub auth flow to also
list any unverified emails that have an associated Zulip account in
the organization.
The trailing slash has no good reason to be there and is also
inconsistent with how we instruct to set up Audience Restriction in the
Okta SAML setup docs for the dev environment.
This new type eliminates a bunch of messy code that previously
involved passing around long lists of mixed positional keyword and
arguments, instead using a consistent data object for communicating
about the state of an external authentication (constructed in
backends.py).
The result is a significantly more readable interface between
zproject/backends.py and zerver/views/auth.py, though likely more
could be done.
This has the side effect of renaming fields for internally passed
structures from name->full_name, next->redirect_to; this results in
most of the test codebase changes.
Modified by tabbott to add comments and collaboratively rewrite the
initialization logic.
We recently changed our droplet setup such that their
host names no longer include zulipdev.org. This caused
a few things to break.
The particular symptom that this commit fixes is that
we were trying to server static assets from
showell:9991 instead of showell.zulipdev.org:9991,
which meant that you couldn't use the app locally.
(The server would start, but the site's pretty unusable
without static assets.)
Now we rely 100% on `dev_settings.py` to set
`EXTERNAL_HOST` for any droplet users who don't set
that var in their own environment. That allows us to
remove some essentially duplicate code in `run-dev.py`.
We also set `IS_DEV_DROPLET` explicitly, so that other
code doesn't have to make inferences or duplicate
logic to detemine whether we're a droplet or not.
And then in `settings.py` we use `IS_DEV_DROPLET` to
know that we can use a prod-like method of calculating
`STATIC_URL`, instead of hard coding `localhost`.
We may want to iterate on this further--this was
sort of a quick fix to get droplets functional again.
It's possible we can re-configure droplets to have
folks get reasonable `EXTERNAL_HOST` settings in their
bash profiles, or something like that, although that
may have its own tradeoffs.
Generated by autopep8 --aggressive, with the setup.cfg configuration
from #14532. In general, an isinstance check may not be equivalent to
a type check because it includes subtypes; however, that’s usually
what you want.
Signed-off-by: Anders Kaseorg <anders@zulipchat.com>
Generated by autopep8, with the setup.cfg configuration from #14532.
I’m not sure why pycodestyle didn’t already flag these.
Signed-off-by: Anders Kaseorg <anders@zulipchat.com>
This is be useful for the mobile and desktop apps to hand an uploaded
file off to the system browser so that it can render PDFs (Etc.).
The S3 backend implementation is simple; for the local upload backend,
we use Django's signing feature to simulate the same sort of 60-second
lifetime token.
Co-Author-By: Mateusz Mandera <mateusz.mandera@protonmail.com>
If SAML_REQUIRE_LIMIT_TO_SUBDOMAINS is enabled, the configured IdPs will
be validated and cleaned up when the saml backend is initialized.
settings.py would be a tempting and more natural place to do this
perhaps, but in settings.py we don't do logging and we wouldn't be able
to write a test for it.
Through the limit_to_subdomains setting on IdP dicts it's now possible
to limit the IdP to only allow authenticating to the specified realms.
Fixes#13340.
This defends against cross-origin session fixation attacks. Renaming
the cookies means this one-time upgrade will have the unfortunate side
effect of logging everyone out, but they’ll get more secure sessions
in return.
Signed-off-by: Anders Kaseorg <anders@zulipchat.com>
Instead of sneakily injecting HttpOnly into the cookie via the path
setting, use the setting that was designed for this purpose.
Signed-off-by: Anders Kaseorg <anders@zulipchat.com>
Since commit 1d72629dc4, we have been
maintaining a patched copy of Django’s
SessionMiddleware.process_response in order to unconditionally ignore
our own optional cookie_domain setting that we don’t set.
Instead, let’s not do that.
Signed-off-by: Anders Kaseorg <anders@zulipchat.com>
Generated by `pyupgrade --py3-plus --keep-percent-format` on all our
Python code except `zthumbor` and `zulip-ec2-configure-interfaces`,
followed by manual indentation fixes.
Signed-off-by: Anders Kaseorg <anders@zulipchat.com>
The information used to be stored in a request._ratelimit dict, but
there's no need for that, and a list is a simpler structure, so this
allows us to simplify the plumbing somewhat.
In development env, we use `get_secret` to get
`SOCIAL_AUTH_GITLAB_KEY` from `dev-secrets.conf`. But in
production env, we don't need this as we ask the user
to set that value in `prod_settings_template.py`.
This restricts the code from looking `zulip-secrets.conf`
for `social_auth_gitlab_key` in production env.
type().__name__ is sufficient, and much readable than type(), so it's
better to use the former for keys.
We also make the classes consistent in forming the keys in the format
type(self).__name__:identifier and adjust logger.warning and statsd to
take advantage of that and simply log the key().
The previous model for GitHub authentication was as follows:
* If the user has only one verified email address, we'll generally just log them in to that account
* If the user has multiple verified email addresses, we will always
prompt them to pick which one to use, with the one registered as
"primary" in GitHub listed at the top.
This change fixes the situation for users going through a "login" flow
(not registration) where exactly one of the emails has an account in
the Zulip oragnization -- they should just be logged in.
Fixes part of #12638.
URLs for config errors were configured seperately for each error
which is better handled by having error name as argument in URL.
A new view `config_error_view` is added containing context for
each error that returns `config_error` page with the relevant
context.
Also fixed tests and some views in `auth.py` to be consistent with
changes.
We had a bunch of ugly hacks to monkey patch things due to upstream
being temporarily unmaintained and not merging PRs. Now the project is
active again and the fixes have been merged and included in the latest
version - so we clean up all that code.
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.
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.
We need to request access to read:org scope to be able to check org/team
membership. Without it SOCIAL_AUTH_GITHUB_ORG_NAME and
SOCIAL_AUTH_GITHUB_TEAM_ID settings don't work and simply lead to all
auth attempts failing.
Tested manually.
isort 5 knows not to reorder imports across function calls, so this
will stop isort from breaking our code.
Signed-off-by: Anders Kaseorg <anders@zulipchat.com>
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.
Extend the context dictionary with variables `social_backend_name`
and `backend_error` flag which determines if the error should be
shown. Not extended this for ldap, smtp and saml as they have a
different format of block.
SOCIAL_AUTH_SAML_SECURITY_CONFIG["authnRequestsSigned"] override in
settings.py in a previous commit wouldn't work on servers old enough to
not have the SAML settings in their settings.py - due to
SOCIAL_AUTH_SAML_SECURITY_CONFIG being undefined.
This commit fixes that.
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.
Profiling suggests this saves about 600us in the runtime of every GET
/events request attempting to resolve URLs to determine whether we
need to do the APPEND_SLASH behavior.
It's possible that we end up doing the same URL resolution work later
and we're just moving around some runtime, but I think even if we do,
Django probably doesn't do any fancy caching that would mean doing
this query twice doesn't just do twice the work.
In any case, we probably want to extend this behavior to our whole API
because the APPEND_SLASH redirect behavior is essentially a bug there.
That is a more involved refactor, however.
The comment explains this issue, but effectively, the upgrade to
Django 2.x means that Django's built-in django.request logger was
writing to our errors logs WARNING-level data for every 404 and 400
error. We don't consider user errors to be a problem worth
highlighting in that log file.
Django 2.2.x is the next LTS release after Django 1.11.x; I expect
we'll be on it for a while, as Django 3.x won't have an LTS release
series out for a while.
Because of upstream API changes in Django, this commit includes
several changes beyond requirements and:
* urls: django.urls.resolvers.RegexURLPattern has been replaced by
django.urls.resolvers.URLPattern; affects OpenAPI code and related
features which re-parse Django's internals.
https://code.djangoproject.com/ticket/28593
* test_runner: Change number to suffix. Django changed the name in this
ticket: https://code.djangoproject.com/ticket/28578
* Delete now-unnecessary SameSite cookie code (it's now the default).
* forms: urlsafe_base64_encode returns string in Django 2.2.
https://docs.djangoproject.com/en/2.2/ref/utils/#django.utils.http.urlsafe_base64_encode
* upload: Django's File.size property replaces _get_size().
https://docs.djangoproject.com/en/2.2/_modules/django/core/files/base/
* process_queue: Migrate to new autoreload API.
* test_messages: Add an extra query caused by .refresh_from_db() losing
the .select_related() on the Realm object.
* session: Sync SessionHostDomainMiddleware with Django 2.2.
There's a lot more we can do to take advantage of the new release;
this is tracked in #11341.
Many changes by Tim Abbott, Umair Waheed, and Mateusz Mandera squashed
are squashed into this commit.
Fixes#10835.
Since essentially the first use of Tornado in Zulip, we've been
maintaining our Tornado+Django system, AsyncDjangoHandler, with
several hundred lines of Django code copied into it.
The goal for that code was simple: We wanted a way to use our Django
middleware (for code sharing reasons) inside a Tornado process (since
we wanted to use Tornado for our async events system).
As part of the Django 2.2.x upgrade, I looked at upgrading this
implementation to be based off modern Django, and it's definitely
possible to do that:
* Continue forking load_middleware to save response middleware.
* Continue manually running the Django response middleware.
* Continue working out a hack involving copying all of _get_response
to change a couple lines allowing us our Tornado code to not
actually return the Django HttpResponse so we can long-poll. The
previous hack of returning None stopped being viable with the Django 2.2
MiddlewareMixin.__call__ implementation.
But I decided to take this opportunity to look at trying to avoid
copying material Django code, and there is a way to do it:
* Replace RespondAsynchronously with a response.asynchronous attribute
on the HttpResponse; this allows Django to run its normal plumbing
happily in a way that should be stable over time, and then we
proceed to discard the response inside the Tornado `get()` method to
implement long-polling. (Better yet might be raising an
exception?). This lets us eliminate maintaining a patched copy of
_get_response.
* Removing the @asynchronous decorator, which didn't add anything now
that we only have one API endpoint backend (with two frontend call
points) that could call into this. Combined with the last bullet,
this lets us remove a significant hack from our
never_cache_responses function.
* Calling the normal Django `get_response` method from zulip_finish
after creating a duplicate request to process, rather than writing
totally custom code to do that. This lets us eliminate maintaining
a patched copy of Django's load_middleware.
* Adding detailed comments explaining how this is supposed to work,
what problems we encounter, and how we solve various problems, which
is critical to being able to modify this code in the future.
A key advantage of these changes is that the exact same code should
work on Django 1.11, Django 2.2, and Django 3.x, because we're no
longer copying large blocks of core Django code and thus should be
much less vulnerable to refactors.
There may be a modest performance downside, in that we now run both
request and response middleware twice when longpolling (once for the
request we discard). We may be able to avoid the expensive part of
it, Zulip's own request/response middleware, with a bit of additional
custom code to save work for requests where we're planning to discard
the response. Profiling will be important to understanding what's
worth doing here.
This makes it possible to create a Zulip account from the mobile or
desktop apps and have the end result be that the user is logged in on
their mobile device.
We may need small changes in the desktop and/or mobile apps to support
this.
Closes#10859.
This adds a new API endpoint for querying basic data on a single other
user in the organization, reusing the existing infrastructure (and
view function!) for getting data on all users in an organization.
Fixes#12277.
https://github.com/Bouke/django-two-factor-auth/issues/297
This setting was added in 1.9 version of the app and can be used
harmleslly in our current Django 1.11-based code and will prevent an
error on Django 2.1+ when we move there.
This is required for our migration to Django 2.2. authenticate()
definitions need to have that starting with Django 2.1.
rate_limit_auth needs to be adjusted to expect the request in the first
positional argument instead of a kwarg.
This makes the state cleaner for the tests. Tests that want to have rate
limiting set up their own desired rules anyway, and having some
pre-existing ones from the default settings can conflict with the
desired ones.
This applies rate limiting (through a decorator) of authenticate()
functions in the Email and LDAP backends - because those are the ones
where we check user's password.
The limiting is based on the username that the authentication is
attempted for - more than X attempts in Y minutes to a username is not
permitted.
If the limit is exceeded, RateLimited exception will be raised - this
can be either handled in a custom way by the code that calls
authenticate(), or it will be handled by RateLimitMiddleware and return
a json_error as the response.
validate_otp_params needs to be moved to backends.py, because as of this
commit it'll be used both there and in views.auth - and import from
views.auth to backends.py causes circular import issue.
Tests require adjusting, because the class-based view has an additional
redirect - through /uid/set-password/ and the token is read from the
session. See Django code of PasswordResetConfirmView.
"Zulip Voyager" was a name invented during the Hack Week to open
source Zulip for what a single-system Zulip server might be called, as
a Star Trek pun on the code it was based on, "Zulip Enterprise".
At the time, we just needed a name quickly, but it was never a good
name, just a placeholder. This removes that placeholder name from
much of the codebase. A bit more work will be required to transition
the `zulip::voyager` Puppet class, as that has some migration work
involved.
This legacy cross-realm bot hasn't been used in several years, as far
as I know. If we wanted to re-introduce it, I'd want to implement it
as an embedded bot using those common APIs, rather than the totally
custom hacky code used for it that involves unnecessary queue workers
and similar details.
Fixes#13533.
Because of how login_or_register_remote_user code is structured, this
doesn't change how the flow will go, but it's not a clean use of
login_or_register_remote_user to call it with is_signup=True if sign up
shouldn't actually happen - and may be fragile when refactoring
login_or_register_remote_user.
Zulip has had a small use of WebSockets (specifically, for the code
path of sending messages, via the webapp only) since ~2013. We
originally added this use of WebSockets in the hope that the latency
benefits of doing so would allow us to avoid implementing a markdown
local echo; they were not. Further, HTTP/2 may have eliminated the
latency difference we hoped to exploit by using WebSockets in any
case.
While we’d originally imagined using WebSockets for other endpoints,
there was never a good justification for moving more components to the
WebSockets system.
This WebSockets code path had a lot of downsides/complexity,
including:
* The messy hack involving constructing an emulated request object to
hook into doing Django requests.
* The `message_senders` queue processor system, which increases RAM
needs and must be provisioned independently from the rest of the
server).
* A duplicate check_send_receive_time Nagios test specific to
WebSockets.
* The requirement for users to have their firewalls/NATs allow
WebSocket connections, and a setting to disable them for networks
where WebSockets don’t work.
* Dependencies on the SockJS family of libraries, which has at times
been poorly maintained, and periodically throws random JavaScript
exceptions in our production environments without a deep enough
traceback to effectively investigate.
* A total of about 1600 lines of our code related to the feature.
* Increased load on the Tornado system, especially around a Zulip
server restart, and especially for large installations like
zulipchat.com, resulting in extra delay before messages can be sent
again.
As detailed in
https://github.com/zulip/zulip/pull/12862#issuecomment-536152397, it
appears that removing WebSockets moderately increases the time it
takes for the `send_message` API query to return from the server, but
does not significantly change the time between when a message is sent
and when it is received by clients. We don’t understand the reason
for that change (suggesting the possibility of a measurement error),
and even if it is a real change, we consider that potential small
latency regression to be acceptable.
If we later want WebSockets, we’ll likely want to just use Django
Channels.
Signed-off-by: Anders Kaseorg <anders@zulipchat.com>
With LDAP authentication, we don't currently have a good way to
support the default stream groups feature.
The old behavior was just to assume a user select every default stream
group, which seems wrong; since we didn't prompt the user about these,
we should just ignore the feature.
These are leftovers from where we had default settings in the
settings.py file. Now that the files are separate those references to
"below" are not correct.
Our ldap integration is quite sensitive to misconfigurations, so more
logging is better than less to help debug those issues.
Despite the following docstring on ZulipLDAPException:
"Since this inherits from _LDAPUser.AuthenticationFailed, these will
be caught and logged at debug level inside django-auth-ldap's
authenticate()"
We weren't actually logging anything, because debug level messages were
ignored due to our general logging settings. It is however desirable to
log these errors, as they can prove useful in debugging configuration
problems. The django_auth_ldap logger can get fairly spammy on debug
level, so we delegate ldap logging to a separate file
/var/log/zulip/ldap.log to avoid spamming server.log too much.
A block of LDAP integration code related to data synchronization did
not correctly handle EMAIL_ADDRESS_VISIBILITY_ADMINS, as it was
accessing .email, not .delivery_email, both for logging and doing the
mapping between email addresses and LDAP users.
Fixes#13539.
This simplifies the RDS installation process to avoid awkwardly
requiring running the installer twice, and also is significantly more
robust in handling issues around rerunning the installer.
Finally, the answer for whether dictionaries are missing is available
to Django for future use in warnings/etc. around full-text search not
being great with this configuration, should they be required.
Alex Vandiver
Dinesh
Aman
Anders Kaseorg
Tim Abbott
Mohit Gupta
savish
Mateusz Mandera
sahil839
Clara Dantas
Hemanth V. Alluri
Steve Howell
Vishnu KS
Rohitt Vashishtha
arpit551
Tim April
Greg Price
Brainrecursion
wowol
Felix
Anders Kaseorg
Kartik Srivastava
Sahil Batra
Mateusz Mandera
Tim Abbott
Steve Howell
Stefan Weil
Dinesh
rht