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