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).
The previous logic made it look like catching ZulipLDAPException on
the authenticate() line was possible, but it isn't, because that
exception is actually being handled inside django-auth-ldap's
authenticate method.
Previously, if you had LDAPAuthBackend enabled, we basically blocked
any other auth backends from working at all, by requiring the user's
login flow include verifying the user's LDAP password.
We still want to enforce that in the case that the account email
matches LDAP_APPEND_DOMAIN, but there's a reasonable corner case:
Having effectively guest users from outside the LDAP domain.
We don't want to allow creating a Zulip-level password for a user
inside the LDAP domain, so we still verify the LDAP password in that
flow, but if the email is allowed to register (due to invite or
whatever) but is outside the LDAP domain for the organization, we
allow it to create an account and set a password.
For the moment, this solution only covers EmailAuthBackend. It's
likely that just extending the list of other backends we check for in
the new conditional on `email_auth_backend` would be correct, but we
haven't done any testing for those cases, and with auth code paths,
it's better to disallow than allow untested code paths.
Fixes#9422.
Previously, if both EmailAuthBackend and LDAPAuthBackend were enabled,
LDAP users could set a password using EmailAuthBackend and continue to
use that password, even if their LDAP account was later deactivated.
That configuration wasn't supported at all before, so this doesn't fix
a pre-existing security issue, but now that we're making that a valid
configuration, we need to cover this case.
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 is done mainly because this backend has the simplest code path
for calling login_or_register_remote_user, more than because we expect
this case to come up. It'll make it easier to write unit tests for
the `invalid_subdomain` corner case.
In this commit we start to support redirects to urls supplied as a
'next' param for the following two backends:
* GoogleOAuth2 based backend.
* GitHubAuthBackend.
This is necessary for mobile apps to do the right thing when only
RemoteUserBackend is enabled, namely, directly redirect to the
third-party SSO auth site as soon as the user enters the server URL
(no need to display a login form, since it'll be useless).
I probably should have just done this in the original implementation;
there's only a small downside in the form of an extra database query
when trying to authenticate a user who doesn't exist.
Structurally, the main change here is replacing the `clean_username`
function, which would get called when one accessed
self.cleaned_data['username'] with code in the main `clean` function.
This is important because only in `clean` do we have access to the
`realm` object.
Since I recently added full test coverage on this form, we know each
of the major cases have a test; the error messages are unchanged.
This deletes the old mock-covered test for this, which was mostly
useless. We have a much less messy test, which we extend to provide
the same test coverage the old one did.
While the result was the same before, this makes it more obvious.
We created this redundant pair of conditionals in a preceding commit,
in order to match the indentation of an `except` block so as to slice
the diffs extra finely as we're refactoring auth code.
This lets us delete some duplicate code, since common_get_active_user
handles an account in the wrong subdomain for us.
Also lets us delete the now-unused common_get_active_user_by_email.
This way, we don't attempt to evaluate whether the user's account is
active (etc.) until after we've checked the backend is enabled. This
won't change the result of actual auth, but feels more readable.
This is a behavior change, though we don't check the value in the
caller regardless. It just seems more logical for us to correctly
report to the caller whether the Google auth itself was valid
unconditionally.
This code path was only required because we had remote_user set as a
positional argument here, and thus we'd be running this auth backend's
code when actually using another auth backend (due to how Django auth
backends are selected based on argument signature).
This require some care to ensure we still provide the same nice error
messages for the case of a user who has an account, just not with this
organization.
Also, we fix the fact that the docstring was (and I think always has
been) at best confusing and perhaps even inaccurate.
Tim Abbott
Umair Khan
Aditya Bansal
rht