2fa: Verify 2FA authentication status with is_2fa_verified.

This replaces user.is_verified with is_2fa_verified.

The helper does extra checks such that the user being checked for 2fa
authentication status is valid.

`request.user.is_verified` is functionally the same as `is_verified`
from `django_otp.middleware`, except that the former is monkey-patched
onto the user object by the 2FA middleware. We use the latter wrapped
in `is_2fa_verified` instead to avoid accessing the patched attribute.

See also: 6b24d56e59/docs/source/overview.rst (authentication-and-verification)

Signed-off-by: Zixuan James Li <p359101898@gmail.com>

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

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)

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()

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)