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

@@ -39,4 +39,3 @@ secret-key
 *.mo
 
 venv/
-dal/ldap_max_uid_file

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

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

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

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