realm_settings: Improve authentication_methods param validation.

The endpoint was lacking validation that the authentication_methods dict
submitted by the user made sense. So e.g. it allowed submitting a
nonsense key like NoSuchBackend or modifying the realm's configured
authentication methods for a backend that's not enabled on the server,
which should not be allowed.

Both were ultimately harmless, because:
1. Submitting NoSuchBackend would luckily just trigger a KeyError inside
   the transaction.atomic() block in do_set_realm_authentication_methods
   so it would actually roll back the database changes it was trying to
   make. So this couldn't actually create some weird
   RealmAuthenticationMethod entries.
2. Silently enabling or disabling e.g. GitHub for a realm when GitHub
   isn't enabled on the server doesn't really change anything. And this
   action is only available to the realm's admins to begin with, so
   there's no attack vector here.

test_supported_backends_only_updated wasn't actually testing anything,
because the state it was asserting:
```
        self.assertFalse(github_auth_enabled(realm))
        self.assertTrue(dev_auth_enabled(realm))
        self.assertFalse(password_auth_enabled(realm))
```

matched the desired state submitted to the API...
```
        result = self.client_patch(
            "/json/realm",
            {
                "authentication_methods": orjson.dumps(
                    {"Email": False, "Dev": True, "GitHub": False}
                ).decode()
            },
        )
```

so we just replace it with a new test that tests the param validation.
This commit is contained in:
Mateusz Mandera 2024-02-02 23:32:27 +01:00 committed by Tim Abbott
parent 6e9b25d993
commit 6dd6fc045f
3 changed files with 49 additions and 16 deletions

View File

@ -5,12 +5,14 @@ from typing import Any, Dict, Literal, Optional, Tuple, Union
from django.conf import settings
from django.db import transaction
from django.utils.timezone import now as timezone_now
from django.utils.translation import gettext as _
from confirmation.models import Confirmation, create_confirmation_link, generate_key
from zerver.actions.custom_profile_fields import do_remove_realm_custom_profile_fields
from zerver.actions.message_delete import do_delete_messages_by_sender
from zerver.actions.user_groups import update_users_in_full_members_system_group
from zerver.actions.user_settings import do_delete_avatar_image
from zerver.lib.exceptions import JsonableError
from zerver.lib.message import parse_message_time_limit_setting, update_first_visible_message_id
from zerver.lib.retention import move_messages_to_archive
from zerver.lib.send_email import FromAddress, send_email_to_admins
@ -208,6 +210,19 @@ def parse_and_set_setting_value_if_required(
return parsed_value, setting_value_changed
def validate_authentication_methods_dict_from_api(
realm: Realm, authentication_methods: Dict[str, bool]
) -> None:
current_authentication_methods = realm.authentication_methods_dict()
for name in authentication_methods:
if name not in current_authentication_methods:
raise JsonableError(
_("Invalid authentication method: {name}. Valid methods are: {methods}").format(
name=name, methods=sorted(current_authentication_methods.keys())
)
)
def do_set_realm_authentication_methods(
realm: Realm, authentication_methods: Dict[str, bool], *, acting_user: Optional[UserProfile]
) -> None:

View File

@ -7167,24 +7167,38 @@ class TestAdminSetBackends(ZulipTestCase):
self.assertTrue(password_auth_enabled(realm))
self.assertTrue(dev_auth_enabled(realm))
def test_supported_backends_only_updated(self) -> None:
def test_only_supported_backends_allowed(self) -> None:
# Log in as admin
self.login("desdemona")
# Set some supported and unsupported backends
result = self.client_patch(
"/json/realm",
{
"authentication_methods": orjson.dumps(
{"Email": False, "Dev": True, "GitHub": False}
).decode()
},
)
self.assert_json_success(result)
realm = get_realm("zulip")
# Check that unsupported backend is not enabled
self.assertFalse(github_auth_enabled(realm))
self.assertTrue(dev_auth_enabled(realm))
self.assertFalse(password_auth_enabled(realm))
with self.settings(
AUTHENTICATION_BACKENDS=(
"zproject.backends.EmailAuthBackend",
"zproject.backends.DevAuthBackend",
)
):
result = self.client_patch(
"/json/realm",
{
"authentication_methods": orjson.dumps(
# Github is not a supported authentication backend right now.
{"Email": False, "Dev": True, "GitHub": False}
).decode()
},
)
self.assert_json_error(
result, "Invalid authentication method: GitHub. Valid methods are: ['Dev', 'Email']"
)
# Also protects from completely invalid input like a backend name that doesn't exist.
result = self.client_patch(
"/json/realm",
{"authentication_methods": orjson.dumps({"NoSuchBackend": True}).decode()},
)
self.assert_json_error(
result,
"Invalid authentication method: NoSuchBackend. Valid methods are: ['Dev', 'Email']",
)
class EmailValidatorTestCase(ZulipTestCase):

View File

@ -19,6 +19,7 @@ from zerver.actions.realm_settings import (
do_set_realm_signup_notifications_stream,
do_set_realm_user_default_setting,
parse_and_set_setting_value_if_required,
validate_authentication_methods_dict_from_api,
)
from zerver.decorator import require_realm_admin, require_realm_owner
from zerver.forms import check_subdomain_available as check_subdomain
@ -196,8 +197,11 @@ def update_realm(
if authentication_methods is not None:
if not user_profile.is_realm_owner:
raise OrganizationOwnerRequiredError
validate_authentication_methods_dict_from_api(realm, authentication_methods)
if True not in authentication_methods.values():
raise JsonableError(_("At least one authentication method must be enabled."))
if video_chat_provider is not None and video_chat_provider not in {
p["id"] for p in Realm.VIDEO_CHAT_PROVIDERS.values()
}: