From 0cb0344ac8f464ea78b37b3ac952f2cdf54fdda6 Mon Sep 17 00:00:00 2001 From: Sajeyks Date: Fri, 6 Nov 2026 15:03:57 +0300 Subject: [PATCH] Compute the next uidNumber from LDAP instead of a local file The highest uidNumber was cached in dal/ldap_max_uid_file. That breaks with more than one replica and is lost on every container restart. The uniqueness check behind it searched base '' (the root DSE), which OpenLDAP never matches, so nothing caught a stale file. The error path also referenced a setting that does not exist. Now a paged search over ENTIRE_SEARCH_BASE finds the highest uidNumber on each activation. Numbering is unchanged: the next number is above both LDAP_DEFAULT_START_UID and every existing uidNumber. LDAP_MAX_UID_FILE_PATH is no longer used. --- .gitignore | 1 - dal/settings.py | 6 ---- dal/tests/base.py | 12 +------ dal/tests/test_flows.py | 13 +++++++ dal/ungleich_ldap.py | 80 +++++++++++++++-------------------------- 5 files changed, 43 insertions(+), 69 deletions(-) diff --git a/.gitignore b/.gitignore index 38b9c32..712a31c 100644 --- a/.gitignore +++ b/.gitignore @@ -39,4 +39,3 @@ secret-key *.mo venv/ -dal/ldap_max_uid_file diff --git a/dal/settings.py b/dal/settings.py index e39373f..74dc14f 100644 --- a/dal/settings.py +++ b/dal/settings.py @@ -29,12 +29,6 @@ LDAP_CUSTOMER_DN = config('LDAP_CUSTOMER_DN') LDAP_USERS_DN = config('LDAP_USERS_DN') LDAP_CUSTOMER_GROUP_ID = config('LDAP_CUSTOMER_GROUP_ID', cast=int) -LDAP_MAX_UID_FILE_PATH = config( - 'LDAP_MAX_UID_FILE_PATH', - default=os.path.join(os.path.abspath( - os.path.dirname(__file__)), 'ldap_max_uid_file' - ) -) LDAP_DEFAULT_START_UID = config('LDAP_DEFAULT_START_UID', cast=int) # Search union over OUs diff --git a/dal/tests/base.py b/dal/tests/base.py index 3939495..a4a10ab 100644 --- a/dal/tests/base.py +++ b/dal/tests/base.py @@ -10,16 +10,14 @@ They are deliberately black-box: they assert on what a user sees (templates, emails, status codes) and on what ends up in LDAP, so they keep passing while the internals are rewritten during the upgrade. """ -import os import re -import tempfile import uuid from unittest import mock import ldap3 from django.conf import settings from django.core import mail -from django.test import TestCase, override_settings +from django.test import TestCase from dal.ungleich_ldap import LdapManager @@ -60,11 +58,6 @@ class LdapTestCase(TestCase): def setUp(self): ensure_ldap_up() self._created_dns = [] - fd, self.uid_file = tempfile.mkstemp(prefix="ldap_max_uid_") - os.close(fd) - os.unlink(self.uid_file) # app must cope with a missing file - self._settings = override_settings(LDAP_MAX_UID_FILE_PATH=self.uid_file) - self._settings.enable() # OTP server is external; never hit the network from tests. self.otp_post = mock.patch("dal.views.requests.post").start() self.otp_post.return_value = mock.Mock(status_code=201, text="{}") @@ -77,9 +70,6 @@ class LdapTestCase(TestCase): for dn in self._created_dns: conn.delete(dn) conn.unbind() - self._settings.disable() - if os.path.exists(self.uid_file): - os.unlink(self.uid_file) # LDAP helpers --------------------------------------------------------- diff --git a/dal/tests/test_flows.py b/dal/tests/test_flows.py index d61918f..aab0189 100644 --- a/dal/tests/test_flows.py +++ b/dal/tests/test_flows.py @@ -120,6 +120,19 @@ class RegisterTests(LdapTestCase): uids.append(int(self.ldap_entry(data["username"]).uidNumber.value)) self.assertNotEqual(uids[0], uids[1]) + def test_uid_number_is_above_every_existing_one(self): + from .base import admin_conn + self.make_ldap_user(ou="users") # random high uidNumber + conn = admin_conn() + conn.search("dc=ungleich,dc=ch", "(uidNumber=*)", attributes=["uidNumber"]) + highest = max(int(e.uidNumber.value) for e in conn.entries) + conn.unbind() + data = self.form() + self.track(data["username"]) + self.client.post(REGISTER, data) + self.client.get(self.last_mail_link()) + self.assertEqual(int(self.ldap_entry(data["username"]).uidNumber.value), highest + 1) + def test_password_not_in_activation_link_or_db(self): """The plaintext password must never be stored or put in a URL.""" from dal.models import PendingRegistration diff --git a/dal/ungleich_ldap.py b/dal/ungleich_ldap.py index 4d1c0e8..044bc6a 100644 --- a/dal/ungleich_ldap.py +++ b/dal/ungleich_ldap.py @@ -88,25 +88,9 @@ class LdapManager: """Create a customer entry. *password_hash* comes from hash_password().""" if self.username_taken(user): raise Exception("The username {} is already taken.".format(user)) - conn = self.get_admin_conn() - uidNumber = self._get_max_uid() + 1 + uidNumber = self._next_uid_number() logger.debug("uidNumber={uidNumber}".format(uidNumber=uidNumber)) - user_exists = True - while user_exists: - user_exists, _ = self.check_user_exists( - "", - '(&(objectClass=inetOrgPerson)(objectClass=posixAccount)' - '(objectClass=top)(uidNumber={uidNumber}))'.format( - uidNumber=uidNumber - ) - ) - if user_exists: - logger.debug( - "{uid} exists. Trying next.".format(uid=uidNumber) - ) - uidNumber += 1 - logger.debug("{uid} does not exist. Using it".format(uid=uidNumber)) - self._set_max_uid(uidNumber) + conn = self.get_admin_conn() try: uid = user # user.encode("utf-8") conn.add("uid={uid},{customer_dn}".format( @@ -260,40 +244,34 @@ class LdapManager: conn.unbind() return return_val, msg - def _set_max_uid(self, max_uid): + def _next_uid_number(self): """ - a utility function to save max_uid value to a file + Return the next free uidNumber: one above the highest uidNumber in + the whole tree, and above LDAP_DEFAULT_START_UID. - :param max_uid: an integer representing the max uid - :return: - """ - with open(settings.LDAP_MAX_UID_FILE_PATH, 'w+') as handler: - handler.write(str(max_uid)) - - def _get_max_uid(self): - """ - A utility function to read the max uid value that was previously set - - :return: An integer representing the max uid value that was previously - set + Read from LDAP on every call (paged, so server size limits don't cut + it short) instead of a local file, so it works with several replicas + and survives restarts. Two registrations activated at the very same + moment could still pick the same number. """ + highest = settings.LDAP_DEFAULT_START_UID + conn = self.get_admin_conn() try: - with open(settings.LDAP_MAX_UID_FILE_PATH, 'r+') as handler: - try: - return_value = int(handler.read()) - except ValueError as ve: - logger.error( - "Error reading int value from {}. {}" - "Returning default value {} instead".format( - settings.LDAP_MAX_UID_PATH, - str(ve), - settings.LDAP_DEFAULT_START_UID - ) - ) - return_value = settings.LDAP_DEFAULT_START_UID - return return_value - except FileNotFoundError as fnfe: - logger.error("File not found : " + str(fnfe)) - return_value = settings.LDAP_DEFAULT_START_UID - logger.error("So, returning UID={}".format(return_value)) - return return_value + results = conn.extend.standard.paged_search( + search_base=settings.ENTIRE_SEARCH_BASE, + search_filter='(&(objectClass=posixAccount)(uidNumber=*))', + attributes=['uidNumber'], + paged_size=500, + generator=True, + ) + for result in results: + if result.get('type') != 'searchResEntry': + continue + value = result['attributes'].get('uidNumber') + if isinstance(value, list): + value = value[0] if value else None + if value is not None: + highest = max(highest, int(value)) + finally: + conn.unbind() + return highest + 1