From 1800b2c7970ba145cfa5ce8eb2ebbd954c805742 Mon Sep 17 00:00:00 2001 From: Mateusz Mandera Date: Sun, 8 Oct 2023 22:53:54 +0200 Subject: [PATCH] ldap: Tweak AUTH_LDAP_ADVANCED_REALM_ACCESS_CONTROL behavior. The original behavior of this setting was to disable LDAP authentication for any realms not configured to use it. This was an arbitrary choice, and its only value was to potentially help catch typos for users who are lazy about testing their configuration. Since it makes it a very inconvenient to potentially host multiple organizations with different LDAP configurations, remove that behavior. --- docs/overview/changelog.md | 6 ++++++ docs/production/authentication-methods.md | 10 +++++----- zerver/tests/test_auth_backends.py | 5 +++-- zproject/backends.py | 15 ++++++++------- 4 files changed, 22 insertions(+), 14 deletions(-) diff --git a/docs/overview/changelog.md b/docs/overview/changelog.md index 16f84e927f..39cb1e2a66 100644 --- a/docs/overview/changelog.md +++ b/docs/overview/changelog.md @@ -159,6 +159,12 @@ _Released 2023-11-16_ various certain other special patterns are now forbidden. In the unlikely event that existing user groups have names matching these patterns, they will be automatically renamed on upgrade. +- The behavior of the `AUTH_LDAP_ADVANCED_REALM_ACCESS_CONTROL` has + subtly changed. Previously, using this setting at all would block + LDAP authentication in organizations that are configured to use LDAP + authentication but not explicitly configured with advanced access + controls. This behavior was removed to simplify hosting multiple + organizations with different LDAP configuration preferences. ## Zulip Server 7.x series diff --git a/docs/production/authentication-methods.md b/docs/production/authentication-methods.md index 21e6edf4b0..8f69e4ac82 100644 --- a/docs/production/authentication-methods.md +++ b/docs/production/authentication-methods.md @@ -435,9 +435,9 @@ organization's subdomain. The corresponding value is a list of `attribute: value` pair sets such that a user is permitted to access the organization if and only if the `attribute: value` pairs in at least one of these sets match the user's LDAP attributes. If this -setting is enabled, organizations not explicitly configured in this -setting will not be accessible via ldap authentication at all. - +setting is enabled, organizations not explicitly configured in it +will not be affected - they'll allow normal LDAP login, unless restricted +by other settings. This is better illustrated with an example: ``` @@ -457,8 +457,8 @@ AUTH_LDAP_ADVANCED_REALM_ACCESS_CONTROL = { This means that the organization `"zulip"` will be accessible via ldap authentication only for users whose ldap attributes either contain both `department: main` `employeeType: staff` or just `office: -Dallas`. LDAP authentication will always fail for all other -organizations in this configuration. +Dallas`. LDAP authentication will proceed normally for all other +organizations. :::{warning} Restricting access using these mechanisms only affects authentication via LDAP, diff --git a/zerver/tests/test_auth_backends.py b/zerver/tests/test_auth_backends.py index d7130594d6..4dffac8bfd 100644 --- a/zerver/tests/test_auth_backends.py +++ b/zerver/tests/test_auth_backends.py @@ -4985,7 +4985,8 @@ class FetchAPIKeyTest(ZulipTestCase): self.assert_json_success(result) self.remove_ldap_user_attr("hamlet", "department") - # Test wrong configuration + # Test a realm that's not configured in the setting. Such a realm should not be affected, + # and just allow normal ldap login. with override_settings( AUTH_LDAP_ADVANCED_REALM_ACCESS_CONTROL={"not_zulip": [{"department": "zulip"}]} ): @@ -4993,7 +4994,7 @@ class FetchAPIKeyTest(ZulipTestCase): "/api/v1/fetch_api_key", dict(username="hamlet", password=self.ldap_password("hamlet")), ) - self.assert_json_error(result, "Your username or password is incorrect", 401) + self.assert_json_success(result) def test_inactive_user(self) -> None: do_deactivate_user(self.user_profile, acting_user=None) diff --git a/zproject/backends.py b/zproject/backends.py index 67653be9c0..fa3397ea0d 100644 --- a/zproject/backends.py +++ b/zproject/backends.py @@ -843,17 +843,18 @@ class ZulipLDAPAuthBackendBase(ZulipAuthMixin, LDAPBackend): # If neither setting is configured, allow access. if realm_access_control is None: return False + if realm.subdomain not in realm_access_control: + # If a realm is not configured in this setting, it shouldn't + # be affected by it - therefore, allow access. + return False # With settings.AUTH_LDAP_ADVANCED_REALM_ACCESS_CONTROL, we # allow access if and only if one of the entries for the # target subdomain matches the user's LDAP attributes. - if not ( - realm.subdomain in realm_access_control - and isinstance(realm_access_control[realm.subdomain], list) - and len(realm_access_control[realm.subdomain]) > 0 - ): - # If configuration is wrong, do not allow access - return True + + # Make sure the format of the setting makes sense. + assert isinstance(realm_access_control[realm.subdomain], list) + assert len(realm_access_control[realm.subdomain]) > 0 # Go through every "or" check for attribute_group in realm_access_control[realm.subdomain]: