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.
We register ZulipRemoteUserBackend as an external_authentication_method
to make it show up in the corresponding field in the /server_settings
endpoint.
This also allows rendering its login button together with
Google/Github/etc. leading to us being able to get rid of some of the
code that was handling it as a special case - the js code for plumbing
the "next" value and the special {% if only_sso %} block in login.html.
An additional consequence of the login.html change is that now the
backend will have it button rendered even if it isn't the only backend
enabled on the server.
This commit builds a more complete concept of an "external
authentication method". Our social backends become a special case of an
external authentication method - but these changes don't change the
actual behavior of social backends, they allow having other backends
(that come from python-social-auth and don't use the social backend
pipeline) share useful code that so far only serviced social backends.
Most importantly, this allows having other backends show up in the
external_authentication_methods field of the /server_settings endpoint,
as well as rendering buttons through the same mechanism as we already
did for social backends.
This moves the creation of dictonaries describing the backend for the
API and button rendering code away into a method, that each backend in
this category is responsible for defining.
To register a backend as an external_authentication_method, it should
subclass ExternalAuthMethod and define its dict_representation
classmethod, and finally use the external_auth_method class decorator to
get added to the EXTERNAL_AUTH_METHODS list.
The problem was that, for example, given a configuration of social
backend + LDAPPopulator, if a user that's not in ldap was being
registered, the Full Name field in the registration form would be
empty instead of getting prefilled with the name provided by the
social backend.
This fixes it - first we try to get the name from ldap. If that
succeeds, a form is created pre-filled with that name. Otherwise, we
proceed to attempt to pre-fill with other means.
This also has a nice side effect of reorganizing most of the logic to
be more parallel between LDAP and other sources of name data.
Previously, the LDAP code for syncing user data was not
multiple-realm-aware, resulting in errors trying to sync data for an
LDAP user present in multiple realms.
Tweaked by tabbott to add some extended comments.
Fixes#11520.
For a long time, we've been only doing the zxcvbn password strength
checks on the browser, which is helpful, but means users could through
hackery (or a bug in the frontend validation code) manage to set a
too-weak password. We fix this by running our password strength
validation on the backend as well, using python-zxcvbn.
In theory, a bug in python-zxcvbn could result in it producing a
different opinion than the frontend version; if so, it'd be a pretty
bad bug in the library, and hopefully we'd hear about it from users,
report upstream, and get it fixed that way. Alternatively, we can
switch to shelling out to node like we do for KaTeX.
Fixes#6880.
A bug in Zulip's new user signup process meant that users who
registered their account using social authentication (e.g. GitHub or
Google SSO) in an organization that also allows password
authentication could have their personal API key stolen by an
unprivileged attacker, allowing nearly full access to the user's
account.
Zulip versions between 1.7.0 and 2.0.6 were affected.
This commit fixes the original bug and also contains a database
migration to fix any users with corrupt `password` fields in the
database as a result of the bug.
Out of an abundance of caution (and to protect the users of any
installations that delay applying this commit), the migration also
resets the API keys of any users where Zulip's logs cannot prove the
user's API key was not previously stolen via this bug. Resetting
those API keys will be inconvenient for users:
* Users of the Zulip mobile and terminal apps whose API keys are reset
will be logged out and need to login again.
* Users using their personal API keys for any other reason will need
to re-fetch their personal API key.
We discovered this bug internally and don't believe it was disclosed
prior to our publishing it through this commit. Because the algorithm
for determining which users might have been affected is very
conservative, many users who were never at risk will have their API
keys reset by this migration.
To avoid this on self-hosted installations that have always used
e.g. LDAP authentication, we skip resetting API keys on installations
that don't have password authentication enabled. System
administrators on installations that used to have email authentication
enabled, but no longer do, should temporarily enable EmailAuthBackend
before applying this migration.
The migration also records which users had their passwords or API keys
reset in the usual RealmAuditLog table.
The state of the FAKELDAP setup for the dev env has fallen behind the
backend changes and updates to fakeldap (which implemented
SCOPE_ONELEVEL searches), as well as having some other minor issues.
This commit restore it to a working state and now all three config modes
work properly.
Having to account everywhere for both cases of having and not
having email search configured makes things needlessly complicated.
It's better to make the setting obligatory in configurations other than
LDAP_APPEND_DOMAIN.
By adding some additional plumbing (through PreregistrationUser) of the
full_name and an additional full_name_validated option, we
pre-populate the Full Name field in the registration form when coming
through a social backend (google/github/saml/etc.) and potentially skip
the registration form (if the user would have nothing to do there other
than clicking the Confirm button) and just create the account and log
the user in.
The main purpose of this is to make that name change happen in
/server_settings. external_authentication_methods is a much better, more
descriptive name than social_backends from API perspective.
These are returned through the API, at the /server_settings
endpoint. It's better to just return the list of dicts with a guarantee
of being sorted in the correct order, than to clutter things with the
sort_order field.
This legacy endpoint was designed for the original native Zulip mobile
apps, which were deprecated years ago in favor of the React Native
app.
It was replaced by /server_settings for active use years ago, so it's
safe to remove it now.
The url scheme is now /accounts/login/social/saml/{idp_name} to initiate
login using the IdP configured under "idp_name" name.
display_name and display_logo (the name and icon to show on the "Log in
with" button) can be customized by adding the apprioprate settings in
the configured IdP dictionaries.
This changes the way django_to_ldap_username works to make sure the ldap
username it returns actually has a corresponding ldap entry and raise an
exception if that's not possible. It seems to be a more sound approach
than just having it return its best guess - which was the case so far.
Now there is a guarantee that what it returns is the username of an
actual ldap user.
This allows communicating to the registration flow when the email being
registered doesn't belong to ldap, which then will proceed to register
it via the normal email backend flow - finally fixing the bug where you
couldn't register a non-ldap email even with the email backend enabled.
These changes to the behavior of django_to_ldap_username require small
refactorings in a couple of other functions that call it, as well as
adapting some tests to these changes. Finally, additional tests are
added for the above-mentioned registration flow behavior and some
related corner-cases.
Instead of mocking the _LDAPUser class, these tests can now take
advantage of the test directory that other ldap are using. After these
changes, test_query_email_attr also verifies that query_ldap can
successfully be used to query by user email, if email search is
configured.
Fixes#11878
Instead of a confusing mix of django_auth_backed applying
ldap_to_django_username in its internals for one part of the
translation, and then custom logic for grabbing it from the email
attribute of the ldapuser in ZulipLDAPAuthBackend.get_or_build_user
for the second part of the translation,
we put all the logic in a single function user_email_from_ldapuser
which will be used by get_or_build of both ZulipLDAPUserPopulator and
ZulipLDAPAuthBackend.
This, building on the previous commits with the email search feature,
fixes the ldap sync bug from issue #11878.
If we can get upstream django-auth-ldap to merge
https://github.com/django-auth-ldap/django-auth-ldap/pull/154, we'll
be able to go back to using the version of ldap_to_django_username
that accepts a _LDAPUser object.
With this, django_to_ldap_username can take an email and find the ldap
username of the ldap user who has this email - if email search is
configured.
This allows successful authenticate() with ldap email and ldap password,
instead of ldap username. This is especially useful because when
a user wants to fetch their api key, the server attempts authenticate
with user_profile.email - and this used to fail if the user was an ldap
user (because the ldap username was required to authenticate
succesfully). See issue #9277.
This fixes a collection of bugs surrounding LDAP configurations A and
C (i.e. LDAP_APPEND_DOMAIN=None) with EmailAuthBackend also enabled.
The core problem was that our desired security model in that setting
of requiring LDAP authentication for accounts managed by LDAP was not
implementable without a way to
Now admins can configure an LDAPSearch query that will find if there
are users in LDAP that have the email address and
email_belongs_to_ldap() will take advantage of that - no longer
returning True in response to all requests and thus blocking email
backend authentication.
In the documentation, we describe this as mandatory configuration for
users (and likely will make it so soon in the code) because the
failure modes for this not being configured are confusing.
But making that change is pending work to improve the relevant error
messages.
Fixes#11715.
MigrationsTestCase is intentionally omitted from this, since migrations
tests are different in their nature and so whatever setUp()
ZulipTestCase may do in the future, MigrationsTestCase may not
necessarily want to replicate.
There are a few outstanding issues that we expect to resolve beforce
including this in a release, but this is good checkpoint to merge.
This PR is a collaboration with Tim Abbott.
Fixes#716.
Also cleans up the interface between the management command and the
LDAP backends code to not guess/recompute under what circumstances
what should be logged.
Co-authored-by: mateuszmandera <mateusz.mandera@protonmail.com>
The order of operations for our LDAP synchronization code wasn't
correct: We would run the code to sync avatars (etc.) even for
deactivated users.
Thanks to niels for the report.
Co-authored-by: mateuszmandera <mateusz.mandera@protonmail.com>
Fixes#13130.
django_auth_ldap doesn't give any other way of detecting that LDAPError
happened other than catching the signal it emits - so we have to
register a receiver. In the receiver we just raise our own Exception
which will properly propagate without being silenced by
django_auth_ldap. This will stop execution before the user gets
deactivated.
I changed the class of the title in order to use the same styling as the
other similar pages (like `/accounts/go` or `/login`).
Changed the related test.
Apparently GitHub changed the email address for these; we need to
update our code accordingly.
One cannot receive emails on the username@users.noreply.github.com, so
if someone tries creating an account with this email address, that
person would not be able to verify the account.
Django’s default FileSystemFinder disallows STATICFILES_DIRS from
containing STATIC_ROOT (by raising an ImproperlyConfigured exception),
because STATIC_ROOT is supposed to be the result of collecting all the
static files in the project, not one of the potentially many sources
of static files.
Signed-off-by: Anders Kaseorg <anders@zulipchat.com>
This replaces the two custom Google authentication backends originally
written in 2012 with using the shared python-social-auth codebase that
we already use for the GitHub authentication backend. These are:
* GoogleMobileOauth2Backend, the ancient code path for mobile
authentication last used by the EOL original Zulip Android app.
* The `finish_google_oauth2` code path in zerver/views/auth.py, which
was the webapp (and modern mobile app) Google authentication code
path.
This change doesn't fix any known bugs; its main benefit is that we
get to remove hundreds of lines of security-sensitive semi-duplicated
code, replacing it with a widely trusted, high quality third-party
library.
During the time between when we refactored the GitHub authentication
backend to use SocialAuthBase and now (when we're about to migrate
GoogleAuthBackend to use that code path as well), we accidentally
added some GitHub-specific authentication backend tests to the common
test class.
Fix this by moving them to the GitHub-specific subclass.
Previously, our Github authentication backend just used the user's
primary email address associated with GitHub, which was a reasonable
default, but quite annoying for users who have several email addresses
associated with their GitHub account.
We fix this, by adding a new screen where users can select which of
their (verified) GitHub email addresses to use for authentication.
This is implemented using the "partial" feature of the
python-social-auth pipeline system.
Each email is displayed as a button. Clicking on that button chooses
the email. The email value is stored in a hidden input above the
button. The `primary_email` is displayed on top followed by
`verified_non_primary_emails`. Backend name is also passed as
`backend` to the template, which in our case is GitHub.
Fixes#9876.
Mateusz Mandera
Tim Abbott
Alexandra Ciobica
Anders Kaseorg
Harshit Bansal
Wyatt Hoodes
vinitS101
Shubham Padia