Use read-only types (List ↦ Sequence, Dict ↦ Mapping, Set ↦
AbstractSet) to guard against accidental mutation of the default
value.
Signed-off-by: Anders Kaseorg <anders@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>
ACCESS_TOKEN_URL works a different for apple authentication, so,
we removed and remocked the ACCESS_TOKEN_URL mock in
`register_extra_endpoints` override of apple auth test class.
It is cleaner to have it as generic feature of `social_auth_test`.
So, this commit adds a function that returns token_data_dict that
we had earlier and is called in the ACCESS_TOKEN_URL mock.
This function is overriden in apple auth test class to generate
payload of the format that apple auth expects.
Thanks to Mateusz Mandera for the simple idea.
Co-authored-by: Mateusz Mandera <mateusz.mandera@zulip.com>
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>
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>
mock is just a backport of the standard library’s unittest.mock now.
The SAMLAuthBackendTest change is needed because
MagicMock.call_args.args wasn’t introduced until Python
3.8 (https://bugs.python.org/issue21269).
The PROVISION_VERSION bump is skipped because mock is still an
indirect dev requirement via moto.
Signed-off-by: Anders Kaseorg <anders@zulip.com>
This is important, because lack of this meant that the POST request in
our tests still had the old session, with various params stored in it.
This mechanism doesn't work in reality in SAML, so the backend uses
redis to store and recover the params from redis. Without flushing the
session, these tests would fail to catch some breakages in the
redis-based mechanism.
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.
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.
As "choose email" screen is only used for GitHub auth, the part
that deals with it is separated from `social_auth_test` and
dealt in a new function `social_auth_finish`. This new
`social_auth_finish` contains only the code that deals with
authentication backends that do not have "choose email" screen.
But it is overidden in GitHub test class to handle the
"choose email" screen.
It was refactored because `expect_choose_email_screen` blocks
were confusing while figuring out how tests work on non GitHub
auths.
This does not rely on the desktop app being able to register for the
zulip:// scheme (which is problematic with, for example, the AppImage
format).
It also is a better interface for managing changes to the system,
since the implementation exists almost entirely in the server/webapp
project.
This provides a smoother user experience, where the user doesn't need
to do the paste step, when combined with
https://github.com/zulip/zulip-desktop/pull/943.
Fixes#13613.
Signed-off-by: Anders Kaseorg <anders@zulip.com>
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.
The purpose is to provide a way for (non-webapp) clients,
like the mobile and terminal apps, to tell whether the
server it's talking to is new enough to support a given
API feature -- in particular a way that
* is finer-grained than release numbers, so that for
features developed after e.g. 2.1.0 we can use them
immediately on servers deployed from master (like
chat.zulip.org and zulipchat.com) without waiting the
months until a 2.2 release;
* is reliable, unlike e.g. looking at the number of
commits since a release;
* doesn't lead to a growing bag of named feature flags
which the server has to go on sending forever.
Tweaked by tabbott to extend the documentation.
Closes#14618.
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.
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>
This used to show a blank page. Considering that the links remain valid
only for 15 seconds it's important to show something more informative to
the user.
The function `prepare_login_url_and_headers` returns a register
link for any value of `is_signup` unless it's not none.
This commit changes it to a boolean for that function and other
functions using it so that it becomes much clearer when a
register link will be returned.
Also, all occurrences of `is_signup='1'` are changed to
`is_signup=True` to make the code consistent with the above change.
When a user in login flow using github auth chooses a email that is
not associated with an existing account, it leads to a "continue to
registration" choice. This cannot be tested with the earlier version
of `stage_two_of_registration`.
Also added the test.
Thanks to Mateusz Mandera for the solution.
Co-authored-by: Mateusz Mandera <mateusz.mandera@protonmail.com>
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 try to use the correct variation of `email`
or `delivery_email`, even though in some
databases they are the same.
(To find the differences, I temporarily hacked
populate_db to use different values for email
and delivery_email, and reduced email visibility
in the zulip realm to admins only.)
In places where we want the "normal" realm
behavior of showing emails (and having `email`
be the same as `delivery_email`), we use
the new `reset_emails_in_zulip_realm` helper.
A couple random things:
- I fixed any error messages that were leaking
the wrong email
- a test that claimed to rely on the order
of emails no longer does (we sort user_ids
instead)
- we now use user_ids in some place where we used
to use emails
- for IRC mirrors I just punted and used
`reset_emails_in_zulip_realm` in most places
- for MIT-related tests, I didn't fix email
vs. delivery_email unless it was obvious
I also explicitly reset the realm to a "normal"
realm for a couple tests that I frankly just didn't
have the energy to debug. (Also, we do want some
coverage on the normal case, even though it is
"easier" for tests to pass if you mix up `email`
and `delivery_email`.)
In particular, I just reset data for the analytics
and corporate tests.
We specifically give the existing user different
delivery_email and email addresses, to prevent false
positives during the test that checks that users
signing up with an already-existing email get
an error message.
(We also rename the test.)
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 now have this API...
If you really just need to log in
and not do anything with the actual
user:
self.login('hamlet')
If you're gonna use the user in the
rest of the test:
hamlet = self.example_user('hamlet')
self.login_user(hamlet)
If you are specifically testing
email/password logins (used only in 4 places):
self.login_by_email(email, password)
And for failures uses this (used twice):
self.assert_login_failure(email)
The email domain restriction to @zulip.com is annoying in development
environment when trying to test sign up. For consistency, it's best to
have tests use the same default, and the tests that require domain
restriction can be adjusted to set that configuration up for themselves
explicitly.
We were using `code` to pass around messages.
The `code` field is designed to be a code, not
a human-readable message.
It's possible that we don't actually need two
flavors of messages for these type of validations,
but I didn't want to change that yet.
We **definitely** don't need to put two types of
message in the exception, so I fix that. Instead,
I just have the caller ask what level of detail
it needs.
I added a non-verbose message for the case of
system bots.
I removed the non-translated version of the message
for deactivated accounts, which didn't have test
coverage and is slightly more prone to leaking
email info that we don't want to leak.
We are trying to kill off `validate_email`, so
we no longer call it from these tests.
These tests are already kind of low-level in
nature, so testing the more specific helpers
here should be fine.
Note that we also make the third parameter
to `validate_email` non-optional in this commit,
to preserve 100% coverage. This is really just
refactoring noise--we will soon eliminate the
entire function, but I didn't want to do everything
in a huge commit.
This has two goals:
- sets up a future commit to bulk-validate
emails
- the extracted function is more simple,
since it just has errors, and no codes
or deactivated flags
This commit leaves us in a somewhat funny
intermediate state where we have
`action.validate_email` being a glorified
two-line function with strange parameters,
but subsequent commits will clean this up:
- we will eliminate validate_email
- we will move most of the guts of its
other callee to lib/email_validation.py
To be clear, the code is correct here, just
kinda in an ugly, temporarily-disorganized
intermediate state.
There was some duplicated code to test config error pages for
different auths which could be handled with less duplicated code
by adding those functions to `SocialAuthBase`.
Also moving the other tests makes it easier to access tests related
to a backend auth when they are in the same file.
Original idea was that KeyError was only going to happen there in case
of user passing bad input params to the endpoint, so logging a generic
message seemed sufficient. But this can also happen in case of
misconfiguration, so it's worth logging more info as it may help in
debugging the configuration.
Create a new page for desktop auth flow, in which
users can select one from going to the app or
continue the flow in the browser.
Co-authored-by: Mateusz Mandera <mateusz.mandera@protonmail.com>
To avoid some hidden bugs in tests caused by every ldap user having the
same password, we give each user a different password, generated based
on their uids (to avoid some ugly hard-coding in a bunch of places).
This 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 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.
The desktop otp flow (to be added in next commits) will want to generate
one-time tokens for the app that will allow it to obtain an
authenticated session. log_into_subdomain will be the endpoint to pass
the one-time token to. Currently it uses signed data as its input
"tokens", which is not compatible with the otp flow, which requires
simpler (and fixed-length) token. Thus the correct scheme to use is to
store the authenticated data in redis and return a token tied to the
data, which should be passed to the log_into_subdomain endpoint.
In this commit, we replace the "pass signed data around" scheme with the
redis scheme, because there's no point having both.
authenticate_remote_user already takes care of calling the authenticate
with the dummy backend. Also, return_data is not used and catching
DoesNotExist exception is not needed, as the dummy backend just returns
None if user isn't found.
responses is an module analogous to httpretty for mocking external
URLs, with a very similar interface (potentially cleaner in that it
makes use of context managers).
The most important (in the moment) problem with httpretty is that it
breaks the ability to use redis in parts of code where httpretty is
enabled. From more research, the module in general has tendency to
have various troublesome bugs with breaking URLs that it shouldn't be
affecting, caused by it working at the socket interface layer. While
those issues could be fixed, responses seems to be less buggy (based
on both third-party reports like ckan/ckan#4755 and our own experience
in removing workarounds for bugs in httpretty) and is more actively
maintained.
Previously, if you tried to invite a user whose account had been
deactivated, we didn't provide a clear path forward for reactivating
the users, which was confusing.
We fix this by plumbing through to the frontend the information that
there is an existing user account with that email address in this
organization, but that it's deactivated. For administrators, we
provide a link for how to reactivate the user.
Fixes#8144.
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.
This fixes an issue that caused LDAP synchronization to fail for
avatars. The problem occurred due to the lack of a 'name' attribute
on the BytesIO object that we pass to the upload backend (which is
only used in the S3 backend for computing Content-Type).
Fixes#12411.
Since positional arguments are interpreted differently by different
backends in Django's authentication backend system, it’s safer to
disallow them.
This had been the motivation for previously declaring the parameters
with default values when we were on Python 2, but that was not super
effective because Python has no rule against positional default
arguments and that convention for our authentication backends was
solely enforced by code review.
Signed-off-by: Anders Kaseorg <anders@zulipchat.com>
This cleans up the pattern for how we check which user is logged in
during Zulip's backend unit tests to be much more readable (replacing
the arcane session code that does this check).
Fixes#12273.
When running the test_query_email_attr test in reverse, the test failed
because self._LDAPUser.attrs was being modified and it was being shared
with other tests.
This makes the implementation of `get_realm` consistent with its
declared return type of `Realm` rather than `Optional[Realm]`.
Fixes#12263.
Signed-off-by: Anders Kaseorg <anders@zulipchat.com>
See the comment, but this is a significant performance optimization
for all of our pages using common_context, because this code path is
called more than a dozen times (recursively) by common_context.
Apparently, our invalid realm error page had HTTP status 200, which
could be confusing and in particular broken our mobile app's error
handling for this case.
Previously, the LDAP authentication model ignored the realm-level
settings for who can join a realm. This was sort of reasonable at the
time, because the original LDAP auth was an SSO solution that didn't
allow multiple realms, and so one could fully configure authentication
settings on the LDAP side. But now that we allow multiple realms with
the LDAP backend, one could easily imagine wanting different
restrictions on them, and so it makes sense to add this enforcement.
Earlier the behavior was to raise an exception thereby stopping the
whole sync. Now we log an error message and skip the field. Also
fixes the `query_ldap` command to report missing fields without
error.
Fixes: #11780.
The night logo synchronization on the settings page was perfect, but
the actual display logic had a few problems:
* We were including the realm_logo in context_processors, even though
it is only used in home.py.
* We used different variable names for the templating in navbar.html
than anywhere else the codebase.
* The behavior that the night logo would default to the day logo if
only one was uploaded was not correctly implemented for the navbar
position, either in the synchronization for updates code or the
logic in the navbar.html templates.
For Google auth, the multiuse invite key should be stored in the
csrf_state sent to google along with other values like is_signup,
mobile_flow_otp.
For social auth, the multiuse invite key should be passed as params to
the social-auth backend. The passing of the key is handled by
social_auth pipeline and made available to us when the auth is
completed.
Extracts out common tests so that future social-auth backends can
be tested without duplicating tests. I have been careful to not
change any testing logic.
As part of Google+ being removed, they've eliminated support for the
/plus/v1/people/me endpoint. Replace it with the very similar
/oauth2/v3/userinfo endpoint.
`fakeldap` assumes every attribute to be a multi-value attribute
while making comparison in `_comapare_s()` and so while making
comparisons for password it gives a false positive. The result
of this was that it was possible to login in the dev environment
using LDAP using a substring of the password. For example, if the
LDAP password is `ldapuser1` even entering `u` would log you in.
We had an inconsistent behavior when `LDAP_APPEND_DOMAIN` was set
in that we allowed user to enter username instead of his email in
the auth form but later the workflow failed due to a small bug.
Fixes: #10917.
This adds a new realm_logo field, which is a horizontal-format logo to
be displayed in the top-left corner of the webapp, and any other
places where we might want a wide-format branding of the organization.
Tweaked significantly by tabbott to rebase, fix styling, etc.
Fixing the styling of this feature's loading indicator caused me to
notice the loading indicator for the realm_icon feature was also ugly,
so I fixed that too.
Fixes#7995.
This should make life a lot more convenient for organizations that use
the LDAP integration and have their avatars in LDAP already.
This hasn't been end-to-end tested against LDAP yet, so there may be
some minor revisions, but fundamentally, it works, has automated
tests, and should be easy to maintain.
Fixes#286.
This styles the avatar and username that show when the registering
user is importing their settings from an existing Zulip account.
Tweaked by tabbott to fix the test/linter failures, a bit of styling,
and tag strings for translation.
Apparently, while the main code path through
login_or_register_remote_user was correctly calling
remote_user_to_email(username) to get a proper email address for
situations where auth username != email (i.e. when SSO_APPEND_DOMAIN
is set), we neglected to do so in the mobile_flow_otp corner case.
Fixes#11005.
Removes email_not_verified option. That option was used to assign
email_data a different set of emails for a test. Instead of that,
this refactor allows to specify the email_data itself in the function
which calls github_oauth2_test. Flags like email_not_verified are
generally used in one test. This is a preparatory refactor for
choose email screen which may have introduced multiple flags otherwise.
The email_list returned has the primary email as the first element.
Testing: The order of the emails in the test was changed to put a
verified email before the primary one. The tests would fail without
this commit's change after the changes in the order of test emails.
Fixes part of #10297.
Use FAKE_LDAP_NUM_USERS which specifies the number of LDAP users
instead of FAKE_LDAP_EXTRA_USERS which specified the number of
extra users.
The output of generate_dev_ldap_dir was being tested against the fixture
located at zerver/tests/fixtures/ldap_dir.json. This didn't make much sense
as generate_dev_ldap_dir was itself used by developers to generate/update
the fixtures. Instead, test_generate_dev_ldap_dir checks the structure of
the dict returned by generate_dev_ldap_dir. The structure is checked by
regex checks, checking whether the dict contains some keys or not, etc.
Issue: When you created a new organization with /new, the "new login"
emails were emailed. We previously had a hack of adding the
.just_registered property to the user Python object to attempt to
prevent the emails, and checking that in zerver/signals.py. This
commit gets rid of the .just_registered check.
Instead of the .just_registered check, this checks if the user has
joined more than a minute before.
A test test_dont_send_login_emails_for_new_user_registration_logins
already exists.
Tweaked by tabbott to introduce the constant JUST_CREATED_THRESHOLD.
Fixes#10179.
Generates ldap_dir based on the mode and the no. of extra users.
It supports three modes, 'a', 'b' and 'c', description for which
can be found in prod_settings_templates.py.
Now reading API keys from a user is done with the get_api_key wrapper
method, rather than directly fetching it from the user object.
Also, every place where an action should be done for each API key is now
using get_all_api_keys. This method returns for the moment a single-item
list, containing the specified user's API key.
This commit is the first step towards allowing users have multiple API
keys.
This adds a new settings, SOCIAL_AUTH_SUBDOMAIN, which specifies which
domain should be used for GitHub auth and other python-social-auth
backends.
If one is running a single-realm Zulip server like chat.zulip.org, one
doesn't need to use this setting, but for multi-realm servers using
social auth, this fixes an annoying bug where the session cookie that
python-social-auth sets early in the auth process on the root domain
ends up masking the session cookie that would have been used to
determine a user is logged in. The end result was that logging in
with GitHub on one domain on a multi-realm server like zulipchat.com
would appear to log you out from all the others!
We fix this by moving python-social-auth to a separate subdomain.
Fixes: #9847.
We need to do a small monkey-patching of python-social-auth to ensure
that it doesn't 500 the request when a user does something funny in
their browser (e.g. using the back button in the auth flow) that is
fundamentally a user error, not a server error.
This was present in the pre-rewrite version of our Social auth
codebase, without clear documentation; I've fixed the explanation
part here.
It's perhaps worth investigating with the core social auth team
whether there's a better way to do this.
It's possible to make GitHub social authentication support letting the
user pick which of their verified email addresses to pick, using the
python-social-auth pipeline feature. We need to add an additional
screen to let the user pick, so we're not adding support for that now,
but this at least migrates this to use the data set of all emails that
have been verified as associated with the user's GitHub account (and
we just assume the user wants their primary email).
This also fixes the inability for very old GitHub accounts (where the
`email` field in the details might be a string the user wanted on
their GitHub profile page) to using GitHub auth to login.
Fixes#9127.
This new implementation model is a lot cleaner and should extend
better to the non-oauth backend supported by python-social-auth (since
we're not relying on monkey-patching `do_auth` in the OAuth backend
base class).
This revised GitHub auth backend test is inspired by the end-to-end
flow model of the Google auth backend test. My hope is that we will
be able to migrate the rest of the important cases in the GitHub auth
backend tests to this model and then delete what is now
GitHubAuthBackendLegacyTest.
The next step after that will be to merge the GitHub and Google auth
tests (since actually, the actual test functions are basically
identical between the two).
If a user's account has been deactivated, we want to provide a special
error message that makes clear what's going on.
Future work is to provide some administrative controls on whether a
user should be able to re-activate their account.
A common path is a new user goes to realm_uri, which redirects to
realm_uri/login, and clicks the google auth button thinking it is a
registration button.
This commit just changes the wording on the page they land on to be
friendlier for that use case.
The main change here is to send a proper confirmation link to the
frontend in the `confirm_continue_registration` code path even if the
user didn't request signup, so that we don't need to re-authenticate
the user's control over their email address in that flow.
This also lets us delete some now-unnecessary code: The
`invalid_email` case is now handled by HomepageForm.is_valid(), which
has nice error handling, so we no longer need logic in the context
computation or template for `confirm_continue_registration` for the
corner case where the user somehow has an invalid email address
authenticated.
We split one GitHub auth backend test to now cover both corner cases
(invalid email for realm, and valid email for realm), and rewrite the
Google auth test for this code path as well.
Fixes#5895.
By moving all of the logic related to the is_signup flag into
maybe_send_to_registration, we make the login_or_register_remote_user
function quite clean and readable.
The next step is to make maybe_send_to_registration less of a
disaster.
The code in maybe_send_to_registration incorrectly used the
`get_realm_from_request` function to fetch the subdomain. This usage
was incorrect in a way that should have been irrelevant, because that
function only differs if there's a logged-in user, and in this code
path, a user is never logged in (it's the code path for logged-out
users trying to sign up).
This this bug could confuse unit tests that might run with a logged-in
client session. This made it possible for several of our GitHub auth
tests to have a totally invalid subdomain value (the root domain).
Fixing that bug in the tests, in turn, let us delete a code path in
the GitHub auth backend logic in `backends.py` that is impossible in
production, and had just been left around for these broken tests.
This code path has actually been dead for a while (since
`invalid_subdomain` gets set to True only when `user_profile` is
`None`). We might want to re-introduce it later, but for now, we
eliminate it and the artificial test that provided it with test
coverage.
Anders Kaseorg
Anders Kaseorg
Dinesh
Mateusz Mandera
whoodes
Tim Abbott
Mateusz Mandera
Hashir Sarwar
Stefan Weil
Dinesh
Steve Howell
vsvipul
Tlazypanda
Alexandra Ciobica
Harshit Bansal
vinitS101
Shubham Padia
Mayank Madan
Yashashvi Dave
Shubham Padia
Challa Venkata Raghava Reddy
sahil839
Vishnu Ks
Anders Kaseorg
Joshua Pan
Max Nussenbaum
Jack Zhang
Yago González
Aditya Bansal
Umair Khan
Rishi Gupta