diff --git a/zerver/decorator.py b/zerver/decorator.py index 81b923925e..b837ffbe75 100644 --- a/zerver/decorator.py +++ b/zerver/decorator.py @@ -63,6 +63,7 @@ from zerver.lib.response import json_method_not_allowed, json_success, json_unau from zerver.lib.subdomains import get_subdomain, user_matches_subdomain from zerver.lib.timestamp import datetime_to_timestamp, timestamp_to_datetime from zerver.lib.types import ViewFuncT +from zerver.lib.users import is_2fa_verified from zerver.lib.utils import has_api_key_format, statsd from zerver.models import Realm, UserProfile, get_client, get_user_profile_by_api_key @@ -1035,7 +1036,7 @@ def zulip_otp_required( to :setting:`OTP_LOGIN_URL`. """ - def test(user: UserProfile) -> bool: + def test(user: Union[UserProfile, AnonymousUser]) -> bool: """ :if_configured: If ``True``, an authenticated user with no confirmed OTP devices will be allowed. Also, non-authenticated users will be @@ -1047,7 +1048,7 @@ def zulip_otp_required( return True # User has completed 2FA verification - if user.is_verified(): + if is_2fa_verified(user): return True # This request is unauthenticated (logged-out) access; 2FA is diff --git a/zerver/lib/users.py b/zerver/lib/users.py index 299218600c..764aa6d0e2 100644 --- a/zerver/lib/users.py +++ b/zerver/lib/users.py @@ -5,10 +5,12 @@ from typing import Any, Dict, Iterable, List, Optional, Sequence, TypedDict, Uni import dateutil.parser as date_parser from django.conf import settings +from django.contrib.auth.models import AnonymousUser from django.core.exceptions import ValidationError from django.db.models.query import QuerySet from django.forms.models import model_to_dict from django.utils.translation import gettext as _ +from django_otp.middleware import is_verified from zulip_bots.custom_exceptions import ConfigValidationError from zerver.lib.avatar import avatar_url, get_avatar_field @@ -620,3 +622,17 @@ def get_raw_user_data( def get_active_bots_owned_by_user(user_profile: UserProfile) -> QuerySet[UserProfile]: return UserProfile.objects.filter(is_bot=True, is_active=True, bot_owner=user_profile) + + +def is_2fa_verified(user: Union[UserProfile, AnonymousUser]) -> bool: + """ + It is generally unsafe to call is_verified directly on `request.user` since + the attribute `otp_device` does not exist on an `AnonymousUser`, and `is_verified` + does not make sense without 2FA being enabled. + + This wraps the checks for all these assumptions to make sure the call is safe. + """ + # Explicitly require the caller to ensure that settings.TWO_FACTOR_AUTHENTICATION_ENABLED + # is True before calling `is_2fa_verified`. + assert settings.TWO_FACTOR_AUTHENTICATION_ENABLED + return user.is_authenticated and is_verified(user) diff --git a/zerver/tests/test_decorators.py b/zerver/tests/test_decorators.py index 2366604e70..6d64a9946a 100644 --- a/zerver/tests/test_decorators.py +++ b/zerver/tests/test_decorators.py @@ -1849,19 +1849,18 @@ class TestZulipLoginRequiredDecorator(ZulipTestCase): "PATH_INFO": "", } user = hamlet = self.example_user("hamlet") - user.is_verified = lambda: False self.login_user(hamlet) request = HostRequestMock( client_name="", user_profile=user, meta_data=meta_data, host="zulip.testserver" ) request.session = self.client.session - response = test_view(request) - self.assertEqual(response.content.decode(), "Success") + with mock.patch("zerver.lib.users.is_verified", lambda _: False): + response = test_view(request) + self.assertEqual(response.content.decode(), "Success") with self.settings(TWO_FACTOR_AUTHENTICATION_ENABLED=True): user = hamlet = self.example_user("hamlet") - user.is_verified = lambda: False self.login_user(hamlet) request = HostRequestMock( client_name="", user_profile=user, meta_data=meta_data, host="zulip.testserver" @@ -1870,7 +1869,8 @@ class TestZulipLoginRequiredDecorator(ZulipTestCase): assert type(request.user) is UserProfile self.create_default_device(request.user) - response = test_view(request) + with mock.patch("zerver.lib.users.is_verified", lambda _: False): + response = test_view(request) self.assertEqual(response.status_code, 302) @@ -1889,7 +1889,6 @@ class TestZulipLoginRequiredDecorator(ZulipTestCase): "PATH_INFO": "", } user = hamlet = self.example_user("hamlet") - user.is_verified = lambda: True self.login_user(hamlet) request = HostRequestMock( client_name="", user_profile=user, meta_data=meta_data, host="zulip.testserver" @@ -1898,8 +1897,9 @@ class TestZulipLoginRequiredDecorator(ZulipTestCase): assert type(request.user) is UserProfile self.create_default_device(request.user) - response = test_view(request) - self.assertEqual(response.content.decode(), "Success") + with mock.patch("zerver.lib.users.is_verified", lambda _: True): + response = test_view(request) + self.assertEqual(response.content.decode(), "Success") def test_otp_not_authenticated(self) -> None: @zulip_otp_required() diff --git a/zerver/views/auth.py b/zerver/views/auth.py index 7660497d33..0afa60e775 100644 --- a/zerver/views/auth.py +++ b/zerver/views/auth.py @@ -67,7 +67,7 @@ from zerver.lib.subdomains import get_subdomain, is_subdomain_root_or_alias from zerver.lib.types import ViewFuncT from zerver.lib.url_encoding import append_url_query_string from zerver.lib.user_agent import parse_user_agent -from zerver.lib.users import get_api_key +from zerver.lib.users import get_api_key, is_2fa_verified from zerver.lib.utils import has_api_key_format from zerver.lib.validator import validate_login_email from zerver.models import ( @@ -760,7 +760,7 @@ def login_page( # logged-in app. is_preview = "preview" in request.GET if settings.TWO_FACTOR_AUTHENTICATION_ENABLED: - if request.user and request.user.is_verified(): + if request.user and is_2fa_verified(request.user): return HttpResponseRedirect(request.user.realm.uri) elif request.user.is_authenticated and not is_preview: return HttpResponseRedirect(request.user.realm.uri)