auth: Add hardening authenticate(use_dummy_backend=True) in do_login.

As explained in the comment, this is to prevent bugs where some strange
combination of codepaths could end up calling do_login without basic
validation of e.g. the subdomain. The usefulness of this will be
extended with the upcoming commit to add the ability to configure custom
code to wrap authenticate() calls in. This will help ensure that some
codepaths don't slip by the mechanism, ending up logging in a user
without the chance for the custom wrapper to run its code.

(cherry picked from commit 72bea3433e)
This commit is contained in:
Mateusz Mandera 2024-01-08 22:19:08 +01:00 committed by Tim Abbott
parent 5782caed97
commit fc85d7d613
3 changed files with 92 additions and 18 deletions

View File

@ -19,7 +19,7 @@ from urllib.parse import urlsplit
import django_otp import django_otp
from django.conf import settings from django.conf import settings
from django.contrib.auth import REDIRECT_FIELD_NAME from django.contrib.auth import REDIRECT_FIELD_NAME, authenticate
from django.contrib.auth import login as django_login from django.contrib.auth import login as django_login
from django.contrib.auth.decorators import user_passes_test as django_user_passes_test from django.contrib.auth.decorators import user_passes_test as django_user_passes_test
from django.contrib.auth.models import AbstractBaseUser, AnonymousUser from django.contrib.auth.models import AbstractBaseUser, AnonymousUser
@ -36,6 +36,7 @@ from django_otp import user_has_device
from two_factor.utils import default_device from two_factor.utils import default_device
from typing_extensions import Concatenate, ParamSpec from typing_extensions import Concatenate, ParamSpec
from zerver.context_processors import get_valid_realm_from_request
from zerver.lib.exceptions import ( from zerver.lib.exceptions import (
AccessDeniedError, AccessDeniedError,
AnomalousWebhookPayloadError, AnomalousWebhookPayloadError,
@ -463,12 +464,30 @@ def do_login(request: HttpRequest, user_profile: UserProfile) -> None:
"""Creates a session, logging in the user, using the Django method, """Creates a session, logging in the user, using the Django method,
and also adds helpful data needed by our server logs. and also adds helpful data needed by our server logs.
""" """
django_login(request, user_profile)
RequestNotes.get_notes(request).requester_for_logs = user_profile.format_requester_for_logs() # As a hardening measure, pass the user_profile through the dummy backend,
process_client(request, user_profile, is_browser_view=True) # which does the minimal validation that the user is allowed to log in.
# This, and stronger validation, should have already been done by the
# caller, so we raise an AssertionError if this doesn't work as expected.
# This is to prevent misuse of this function, as it would pose a major
# security issue.
realm = get_valid_realm_from_request(request)
validated_user_profile = authenticate(
request=request, username=user_profile.delivery_email, realm=realm, use_dummy_backend=True
)
if validated_user_profile is None or validated_user_profile != user_profile:
raise AssertionError("do_login called for a user_profile that shouldn't be able to log in")
assert isinstance(validated_user_profile, UserProfile)
django_login(request, validated_user_profile)
RequestNotes.get_notes(
request
).requester_for_logs = validated_user_profile.format_requester_for_logs()
process_client(request, validated_user_profile, is_browser_view=True)
if settings.TWO_FACTOR_AUTHENTICATION_ENABLED: if settings.TWO_FACTOR_AUTHENTICATION_ENABLED:
# Log in with two factor authentication as well. # Log in with two factor authentication as well.
do_two_factor_login(request, user_profile) do_two_factor_login(request, validated_user_profile)
def log_view_func( def log_view_func(

View File

@ -5424,7 +5424,12 @@ class TestZulipRemoteUserBackend(DesktopFlowTestingLib, ZulipTestCase):
def test_login_success(self) -> None: def test_login_success(self) -> None:
user_profile = self.example_user("hamlet") user_profile = self.example_user("hamlet")
email = user_profile.delivery_email email = user_profile.delivery_email
with self.settings(AUTHENTICATION_BACKENDS=("zproject.backends.ZulipRemoteUserBackend",)): with self.settings(
AUTHENTICATION_BACKENDS=(
"zproject.backends.ZulipRemoteUserBackend",
"zproject.backends.ZulipDummyBackend",
)
):
result = self.client_get("/accounts/login/sso/", REMOTE_USER=email) result = self.client_get("/accounts/login/sso/", REMOTE_USER=email)
self.assertEqual(result.status_code, 302) self.assertEqual(result.status_code, 302)
self.assert_logged_in_user_id(user_profile.id) self.assert_logged_in_user_id(user_profile.id)
@ -5433,7 +5438,10 @@ class TestZulipRemoteUserBackend(DesktopFlowTestingLib, ZulipTestCase):
username = "hamlet" username = "hamlet"
user_profile = self.example_user("hamlet") user_profile = self.example_user("hamlet")
with self.settings( with self.settings(
AUTHENTICATION_BACKENDS=("zproject.backends.ZulipRemoteUserBackend",), AUTHENTICATION_BACKENDS=(
"zproject.backends.ZulipRemoteUserBackend",
"zproject.backends.ZulipDummyBackend",
),
SSO_APPEND_DOMAIN="zulip.com", SSO_APPEND_DOMAIN="zulip.com",
): ):
result = self.client_get("/accounts/login/sso/", REMOTE_USER=username) result = self.client_get("/accounts/login/sso/", REMOTE_USER=username)
@ -5443,7 +5451,12 @@ class TestZulipRemoteUserBackend(DesktopFlowTestingLib, ZulipTestCase):
def test_login_case_insensitive(self) -> None: def test_login_case_insensitive(self) -> None:
user_profile = self.example_user("hamlet") user_profile = self.example_user("hamlet")
email_upper = user_profile.delivery_email.upper() email_upper = user_profile.delivery_email.upper()
with self.settings(AUTHENTICATION_BACKENDS=("zproject.backends.ZulipRemoteUserBackend",)): with self.settings(
AUTHENTICATION_BACKENDS=(
"zproject.backends.ZulipRemoteUserBackend",
"zproject.backends.ZulipDummyBackend",
)
):
result = self.client_get("/accounts/login/sso/", REMOTE_USER=email_upper) result = self.client_get("/accounts/login/sso/", REMOTE_USER=email_upper)
self.assertEqual(result.status_code, 302) self.assertEqual(result.status_code, 302)
self.assert_logged_in_user_id(user_profile.id) self.assert_logged_in_user_id(user_profile.id)
@ -5462,7 +5475,12 @@ class TestZulipRemoteUserBackend(DesktopFlowTestingLib, ZulipTestCase):
def test_login_failure_due_to_nonexisting_user(self) -> None: def test_login_failure_due_to_nonexisting_user(self) -> None:
email = "nonexisting@zulip.com" email = "nonexisting@zulip.com"
with self.settings(AUTHENTICATION_BACKENDS=("zproject.backends.ZulipRemoteUserBackend",)): with self.settings(
AUTHENTICATION_BACKENDS=(
"zproject.backends.ZulipRemoteUserBackend",
"zproject.backends.ZulipDummyBackend",
)
):
result = self.client_get("/accounts/login/sso/", REMOTE_USER=email) result = self.client_get("/accounts/login/sso/", REMOTE_USER=email)
self.assertEqual(result.status_code, 200) self.assertEqual(result.status_code, 200)
self.assert_logged_in_user_id(None) self.assert_logged_in_user_id(None)
@ -5470,13 +5488,21 @@ class TestZulipRemoteUserBackend(DesktopFlowTestingLib, ZulipTestCase):
def test_login_failure_due_to_invalid_email(self) -> None: def test_login_failure_due_to_invalid_email(self) -> None:
email = "hamlet" email = "hamlet"
with self.settings(AUTHENTICATION_BACKENDS=("zproject.backends.ZulipRemoteUserBackend",)): with self.settings(
AUTHENTICATION_BACKENDS=(
"zproject.backends.ZulipRemoteUserBackend",
"zproject.backends.ZulipDummyBackend",
)
):
result = self.client_get("/accounts/login/sso/", REMOTE_USER=email) result = self.client_get("/accounts/login/sso/", REMOTE_USER=email)
self.assert_json_error_contains(result, "Enter a valid email address.", 400) self.assert_json_error_contains(result, "Enter a valid email address.", 400)
def test_login_failure_due_to_missing_field(self) -> None: def test_login_failure_due_to_missing_field(self) -> None:
with self.settings( with self.settings(
AUTHENTICATION_BACKENDS=("zproject.backends.ZulipRemoteUserBackend",) AUTHENTICATION_BACKENDS=(
"zproject.backends.ZulipRemoteUserBackend",
"zproject.backends.ZulipDummyBackend",
)
), self.assertLogs("django.request", level="ERROR") as m: ), self.assertLogs("django.request", level="ERROR") as m:
result = self.client_get("/accounts/login/sso/") result = self.client_get("/accounts/login/sso/")
self.assertEqual(result.status_code, 500) self.assertEqual(result.status_code, 500)
@ -5488,7 +5514,12 @@ class TestZulipRemoteUserBackend(DesktopFlowTestingLib, ZulipTestCase):
def test_login_failure_due_to_wrong_subdomain(self) -> None: def test_login_failure_due_to_wrong_subdomain(self) -> None:
email = self.example_email("hamlet") email = self.example_email("hamlet")
with self.settings(AUTHENTICATION_BACKENDS=("zproject.backends.ZulipRemoteUserBackend",)): with self.settings(
AUTHENTICATION_BACKENDS=(
"zproject.backends.ZulipRemoteUserBackend",
"zproject.backends.ZulipDummyBackend",
)
):
with mock.patch("zerver.views.auth.get_subdomain", return_value="acme"): with mock.patch("zerver.views.auth.get_subdomain", return_value="acme"):
result = self.client_get( result = self.client_get(
"http://testserver:9080/accounts/login/sso/", REMOTE_USER=email "http://testserver:9080/accounts/login/sso/", REMOTE_USER=email
@ -5499,7 +5530,12 @@ class TestZulipRemoteUserBackend(DesktopFlowTestingLib, ZulipTestCase):
def test_login_failure_due_to_empty_subdomain(self) -> None: def test_login_failure_due_to_empty_subdomain(self) -> None:
email = self.example_email("hamlet") email = self.example_email("hamlet")
with self.settings(AUTHENTICATION_BACKENDS=("zproject.backends.ZulipRemoteUserBackend",)): with self.settings(
AUTHENTICATION_BACKENDS=(
"zproject.backends.ZulipRemoteUserBackend",
"zproject.backends.ZulipDummyBackend",
)
):
with mock.patch("zerver.views.auth.get_subdomain", return_value=""): with mock.patch("zerver.views.auth.get_subdomain", return_value=""):
result = self.client_get( result = self.client_get(
"http://testserver:9080/accounts/login/sso/", REMOTE_USER=email "http://testserver:9080/accounts/login/sso/", REMOTE_USER=email
@ -5513,14 +5549,22 @@ class TestZulipRemoteUserBackend(DesktopFlowTestingLib, ZulipTestCase):
email = user_profile.delivery_email email = user_profile.delivery_email
with mock.patch("zerver.views.auth.get_subdomain", return_value="zulip"): with mock.patch("zerver.views.auth.get_subdomain", return_value="zulip"):
with self.settings( with self.settings(
AUTHENTICATION_BACKENDS=("zproject.backends.ZulipRemoteUserBackend",) AUTHENTICATION_BACKENDS=(
"zproject.backends.ZulipRemoteUserBackend",
"zproject.backends.ZulipDummyBackend",
)
): ):
result = self.client_get("/accounts/login/sso/", REMOTE_USER=email) result = self.client_get("/accounts/login/sso/", REMOTE_USER=email)
self.assertEqual(result.status_code, 302) self.assertEqual(result.status_code, 302)
self.assert_logged_in_user_id(user_profile.id) self.assert_logged_in_user_id(user_profile.id)
@override_settings(SEND_LOGIN_EMAILS=True) @override_settings(SEND_LOGIN_EMAILS=True)
@override_settings(AUTHENTICATION_BACKENDS=("zproject.backends.ZulipRemoteUserBackend",)) @override_settings(
AUTHENTICATION_BACKENDS=(
"zproject.backends.ZulipRemoteUserBackend",
"zproject.backends.ZulipDummyBackend",
)
)
def test_login_mobile_flow_otp_success_email(self) -> None: def test_login_mobile_flow_otp_success_email(self) -> None:
user_profile = self.example_user("hamlet") user_profile = self.example_user("hamlet")
email = user_profile.delivery_email email = user_profile.delivery_email
@ -5568,7 +5612,12 @@ class TestZulipRemoteUserBackend(DesktopFlowTestingLib, ZulipTestCase):
@override_settings(SEND_LOGIN_EMAILS=True) @override_settings(SEND_LOGIN_EMAILS=True)
@override_settings(SSO_APPEND_DOMAIN="zulip.com") @override_settings(SSO_APPEND_DOMAIN="zulip.com")
@override_settings(AUTHENTICATION_BACKENDS=("zproject.backends.ZulipRemoteUserBackend",)) @override_settings(
AUTHENTICATION_BACKENDS=(
"zproject.backends.ZulipRemoteUserBackend",
"zproject.backends.ZulipDummyBackend",
)
)
def test_login_mobile_flow_otp_success_username(self) -> None: def test_login_mobile_flow_otp_success_username(self) -> None:
user_profile = self.example_user("hamlet") user_profile = self.example_user("hamlet")
email = user_profile.delivery_email email = user_profile.delivery_email
@ -5689,7 +5738,10 @@ class TestZulipRemoteUserBackend(DesktopFlowTestingLib, ZulipTestCase):
user_profile = self.example_user("hamlet") user_profile = self.example_user("hamlet")
email = user_profile.delivery_email email = user_profile.delivery_email
with self.settings( with self.settings(
AUTHENTICATION_BACKENDS=("zproject.backends.ZulipRemoteUserBackend",) AUTHENTICATION_BACKENDS=(
"zproject.backends.ZulipRemoteUserBackend",
"zproject.backends.ZulipDummyBackend",
)
): ):
result = self.client_get("/accounts/login/sso/", {"next": next}, REMOTE_USER=email) result = self.client_get("/accounts/login/sso/", {"next": next}, REMOTE_USER=email)
return result return result

View File

@ -931,7 +931,7 @@ class LoginTest(ZulipTestCase):
# seem to be any O(N) behavior. Some of the cache hits are related # seem to be any O(N) behavior. Some of the cache hits are related
# to sending messages, such as getting the welcome bot, looking up # to sending messages, such as getting the welcome bot, looking up
# the alert words for a realm, etc. # the alert words for a realm, etc.
with self.assert_database_query_count(104), self.assert_memcached_count(18): with self.assert_database_query_count(105), self.assert_memcached_count(18):
with self.captureOnCommitCallbacks(execute=True): with self.captureOnCommitCallbacks(execute=True):
self.register(self.nonreg_email("test"), "test") self.register(self.nonreg_email("test"), "test")
@ -3762,6 +3762,7 @@ class UserSignUpTest(ZulipTestCase):
AUTHENTICATION_BACKENDS=( AUTHENTICATION_BACKENDS=(
"zproject.backends.ZulipLDAPAuthBackend", "zproject.backends.ZulipLDAPAuthBackend",
"zproject.backends.EmailAuthBackend", "zproject.backends.EmailAuthBackend",
"zproject.backends.ZulipDummyBackend",
) )
) )
def test_ldap_invite_user_as_admin(self) -> None: def test_ldap_invite_user_as_admin(self) -> None:
@ -3773,6 +3774,7 @@ class UserSignUpTest(ZulipTestCase):
AUTHENTICATION_BACKENDS=( AUTHENTICATION_BACKENDS=(
"zproject.backends.ZulipLDAPAuthBackend", "zproject.backends.ZulipLDAPAuthBackend",
"zproject.backends.EmailAuthBackend", "zproject.backends.EmailAuthBackend",
"zproject.backends.ZulipDummyBackend",
) )
) )
def test_ldap_invite_user_as_guest(self) -> None: def test_ldap_invite_user_as_guest(self) -> None:
@ -3784,6 +3786,7 @@ class UserSignUpTest(ZulipTestCase):
AUTHENTICATION_BACKENDS=( AUTHENTICATION_BACKENDS=(
"zproject.backends.ZulipLDAPAuthBackend", "zproject.backends.ZulipLDAPAuthBackend",
"zproject.backends.EmailAuthBackend", "zproject.backends.EmailAuthBackend",
"zproject.backends.ZulipDummyBackend",
) )
) )
def test_ldap_invite_streams(self) -> None: def test_ldap_invite_streams(self) -> None: