Improve LDAP performances when updating records, especially when many user entries exists

conf/slapd/permission.ldif

@@ -16,9 +16,11 @@ olcAttributeTypes: ( 1.3.6.1.4.1.17953.9.1.3 NAME 'inheritPermission'
   SUP distinguishedName )
 olcAttributeTypes: ( 1.3.6.1.4.1.17953.9.1.4 NAME 'URL'
   DESC 'YunoHost permission main URL'
+  EQUALITY caseExactMatch
   SYNTAX 1.3.6.1.4.1.1466.115.121.1.15{128} SINGLE-VALUE )
 olcAttributeTypes: ( 1.3.6.1.4.1.17953.9.1.5 NAME 'additionalUrls'
   DESC 'YunoHost permission additionnal URL'
+  EQUALITY caseExactMatch
   SYNTAX 1.3.6.1.4.1.1466.115.121.1.15{128} )
 olcAttributeTypes: ( 1.3.6.1.4.1.17953.9.1.6 NAME 'authHeader'
   DESC 'YunoHost application, enable authentication header'

src/permission.py

@@ -583,10 +583,10 @@ def permission_url(
         ldap.update(
             f"cn={permission},ou=permission",
             {
-                "URL": [url] if url is not None else [],
+                "URL": url if url is not None else set(),
                 "additionalUrls": new_additional_urls,
-                "authHeader": [str(auth_header).upper()],
-                "showTile": [str(show_tile).upper()],
+                "authHeader": str(auth_header).upper(),
+                "showTile": str(show_tile).upper(),
             },
         )
     except Exception as e:
@@ -674,9 +674,9 @@ def permission_sync_to_user():
             continue
 
         new_inherited_perms = {
-            "inheritPermission": [
+            "inheritPermission": {
                 f"uid={u},ou=users,dc=yunohost,dc=org" for u in should_be_allowed_users
-            ],
+            },
             "memberUid": should_be_allowed_users,
         }
 
@@ -731,15 +731,15 @@ def _update_ldap_group_permission(
         allowed = [allowed] if not isinstance(allowed, list) else allowed
         # Guarantee uniqueness of values in allowed, which would otherwise make ldap.update angry.
         allowed = set(allowed)
-        update["groupPermission"] = [
+        update["groupPermission"] = {
             "cn=" + g + ",ou=groups,dc=yunohost,dc=org" for g in allowed
-        ]
+        }
 
     if label is not None:
-        update["label"] = [str(label)]
+        update["label"] = str(label)
 
     if protected is not None:
-        update["isProtected"] = [str(protected).upper()]
+        update["isProtected"] = str(protected).upper()
 
     if show_tile is not None:
         if show_tile is True:
@@ -756,7 +756,7 @@ def _update_ldap_group_permission(
                     m18n.n("show_tile_cant_be_enabled_for_regex", permission=permission)
                 )
                 show_tile = False
-        update["showTile"] = [str(show_tile).upper()]
+        update["showTile"] = str(show_tile).upper()
 
     try:
         ldap.update(f"cn={permission},ou=permission", update)

src/settings.py

@@ -246,7 +246,7 @@ def _apply(
             ldap = _get_ldap_interface()
             ldap.update(
                 "cn=admins,ou=sudo",
-                {"sudoOption": ["!authenticate"] if passwordless_sudo else []},
+                {"sudoOption": "!authenticate" if passwordless_sudo else set()},
             )
 
         # First save settings except virtual + default ones

src/user.py

@@ -296,9 +296,9 @@ def user_create(
 
     # Create group for user and add to group 'all_users'
     user_group_create(groupname=username, gid=uid, primary_group=True, sync_perm=False)
-    user_group_update(groupname="all_users", add=username, force=True, sync_perm=True)
     if admin:
-        user_group_update(groupname="admins", add=username, sync_perm=True)
+        user_group_update(groupname="admins", add=username, sync_perm=False)
+    user_group_update(groupname="all_users", add=username, force=True, sync_perm=True)
 
     # Trigger post_user_create hooks
     env_dict = {
@@ -443,23 +443,23 @@ def user_update(
     # Get modifications from arguments
     new_attr_dict = {}
     if firstname:
-        new_attr_dict["givenName"] = [firstname]  # TODO: Validate
-        new_attr_dict["cn"] = new_attr_dict["displayName"] = [
-            (firstname + " " + user["sn"][0]).strip()
-        ]
+        new_attr_dict["givenName"] = firstname  # TODO: Validate
+        new_attr_dict["cn"] = new_attr_dict["displayName"] = (
+            firstname + " " + user["sn"][0]
+        ).strip()
         env_dict["YNH_USER_FIRSTNAME"] = firstname
 
     if lastname:
-        new_attr_dict["sn"] = [lastname]  # TODO: Validate
-        new_attr_dict["cn"] = new_attr_dict["displayName"] = [
-            (user["givenName"][0] + " " + lastname).strip()
-        ]
+        new_attr_dict["sn"] = lastname  # TODO: Validate
+        new_attr_dict["cn"] = new_attr_dict["displayName"] = (
+            user["givenName"][0] + " " + lastname
+        ).strip()
         env_dict["YNH_USER_LASTNAME"] = lastname
 
     if lastname and firstname:
-        new_attr_dict["cn"] = new_attr_dict["displayName"] = [
-            (firstname + " " + lastname).strip()
-        ]
+        new_attr_dict["cn"] = new_attr_dict["displayName"] = (
+            firstname + " " + lastname
+        ).strip()
 
     # change_password is None if user_update is not called to change the password
     if change_password is not None and change_password != "":
@@ -481,13 +481,14 @@ def user_update(
             "admin" if is_admin else "user", change_password
         )
 
-        new_attr_dict["userPassword"] = [_hash_user_password(change_password)]
+        new_attr_dict["userPassword"] = _hash_user_password(change_password)
         env_dict["YNH_USER_PASSWORD"] = change_password
 
     if mail:
         # If the requested mail address is already as main address or as an alias by this user
         if mail in user["mail"]:
-            user["mail"].remove(mail)
+            if mail != user["mail"][0]:
+                user["mail"].remove(mail)
         # Othewise, check that this mail address is not already used by this user
         else:
             try:
@@ -513,7 +514,7 @@ def user_update(
 
             # (c.f. similar stuff as before)
             if mail in user["mail"]:
-                user["mail"].remove(mail)
+                continue
             else:
                 try:
                     ldap.validate_uniqueness({"mail": mail})
@@ -542,33 +543,28 @@ def user_update(
     if add_mailforward:
         if not isinstance(add_mailforward, list):
             add_mailforward = [add_mailforward]
-        for mail in add_mailforward:
-            if mail in user["maildrop"][1:]:
-                continue
-            user["maildrop"].append(mail)
-        new_attr_dict["maildrop"] = user["maildrop"]
+        new_attr_dict["maildrop"] = set(user["maildrop"])
+        new_attr_dict["maildrop"].update(set(add_mailforward))
 
     if remove_mailforward:
         if not isinstance(remove_mailforward, list):
             remove_mailforward = [remove_mailforward]
-        for mail in remove_mailforward:
-            if len(user["maildrop"]) > 1 and mail in user["maildrop"][1:]:
-                user["maildrop"].remove(mail)
-            else:
-                raise YunohostValidationError("mail_forward_remove_failed", mail=mail)
-        new_attr_dict["maildrop"] = user["maildrop"]
+        new_attr_dict["maildrop"] = set(user["maildrop"]) - set(remove_mailforward)
+
+        if len(user["maildrop"]) - len(remove_mailforward) != len(new_attr_dict["maildrop"]):
+            raise YunohostValidationError("mail_forward_remove_failed", mail=mail)
 
     if "maildrop" in new_attr_dict:
         env_dict["YNH_USER_MAILFORWARDS"] = ",".join(new_attr_dict["maildrop"])
 
     if mailbox_quota is not None:
-        new_attr_dict["mailuserquota"] = [mailbox_quota]
+        new_attr_dict["mailuserquota"] = mailbox_quota
         env_dict["YNH_USER_MAILQUOTA"] = mailbox_quota
 
     if loginShell is not None:
         if not shellexists(loginShell) or loginShell not in list_shells():
             raise YunohostValidationError("invalid_shell", shell=loginShell)
-        new_attr_dict["loginShell"] = [loginShell]
+        new_attr_dict["loginShell"] = loginShell
         env_dict["YNH_USER_LOGINSHELL"] = loginShell
 
     if not from_import:
@@ -637,7 +633,8 @@ def user_info(username: str) -> dict[str, str]:
         result_dict["mail-aliases"] = user["mail"][1:]
 
     if len(user["maildrop"]) > 1:
-        result_dict["mail-forward"] = user["maildrop"][1:]
+        user["maildrop"].remove(username)
+        result_dict["mail-forward"] = user["maildrop"]
 
     if "mailuserquota" in user:
         userquota = user["mailuserquota"][0]
@@ -1344,14 +1341,11 @@ def user_group_update(
         logger.info(m18n.n("group_update_aliases", group=groupname))
         new_attr_dict["mail"] = list(set(new_group_mail))
 
-        if new_attr_dict["mail"] and "mailGroup" not in group["objectClass"]:
-            new_attr_dict["objectClass"] = group["objectClass"] + ["mailGroup"]
-        if not new_attr_dict["mail"] and "mailGroup" in group["objectClass"]:
-            new_attr_dict["objectClass"] = [
-                c
-                for c in group["objectClass"]
-                if c != "mailGroup" and c != "mailAccount"
-            ]
+        if new_attr_dict["mail"]:
+            new_attr_dict["objectClass"] = set(group["objectClass"])
+            new_attr_dict["objectClass"].update({"mailGroup"})
+        else:
+            new_attr_dict["objectClass"] = set(group["objectClass"]) - {"mailGroup", "mailAccount"}
 
     if new_attr_dict:
         if not from_import:

src/utils/ldap.py

@@ -71,6 +71,54 @@ def _destroy_ldap_interface():
 
 atexit.register(_destroy_ldap_interface)
 
+
+def modifyModlist_finegrained(old_entry: dict, new_entry: dict) -> list:
+    """
+    Prepare an optimized modification list to give to ldap.modify_ext()
+    """
+    ldif = []
+    for attribute, value in new_entry.items():
+        if not isinstance(value, (set, list)):
+            value = {value}
+        old_value = old_entry.get(attribute, set())
+        if not isinstance(old_value, (set, list)):
+            old_value = {old_value}
+        if value == set(old_value):
+            continue
+
+        if not old_value:
+            ldif.append((ldap.MOD_ADD, attribute, list(value)))
+        elif not value:
+            ldif.append((ldap.MOD_DELETE, attribute, list(old_value)))
+        # Add or/and delete only needed values with unordered set
+        elif isinstance(value, set):
+            values_to_del = set(old_value) - value
+            if values_to_del == set(old_value):
+                ldif.append((ldap.MOD_REPLACE, attribute, list(value)))
+                continue
+            elif values_to_del:
+                ldif.append((ldap.MOD_DELETE, attribute, list(values_to_del)))
+
+            values_to_add = value - set(old_value)
+            if values_to_add:
+                ldif.append((ldap.MOD_ADD, attribute, list(values_to_add)))
+
+        # Add or/and delete only needed values with ordered list
+        else:
+            for i, v in enumerate(value):
+                if i >= len(old_value) or old_value[i] != v:
+                    break
+            if i == 0:
+                ldif.append((ldap.MOD_REPLACE, attribute, value))
+            else:
+                if old_value[i:]:
+                    ldif.append((ldap.MOD_DELETE, attribute, old_value[i:]))
+                if value[i:]:
+                    ldif.append((ldap.MOD_ADD, attribute, value[i:]))
+
+    return ldif
+
+
 URI = "ldapi://%2Fvar%2Frun%2Fslapd%2Fldapi"
 BASEDN = "dc=yunohost,dc=org"
 ROOTDN = "gidNumber=0+uidNumber=0,cn=peercred,cn=external,cn=auth"
@@ -260,10 +308,19 @@ def update(self, rdn, attr_dict, new_rdn=False):
 
         """
         dn = f"{rdn},{BASEDN}"
-        actual_entry = self.search(rdn, attrs=None)
-        ldif = modlist.modifyModlist(actual_entry[0], attr_dict, ignore_oldexistent=1)
+        current_entry = self.search(rdn, attrs=None)
+
+
+        # Previously, we used modifyModlist, which directly uses the lib system libldap
+        # supplied with openldap. Unfortunately, the output of this command was not
+        # optimal with attributes containing lists (complete deletion then complete
+        # rewriting of the list). In view of the major performance problems associated
+        # with our inherited permissions system, we decided to rewrite this part to
+        # optimize the output.
+        # ldif = modlist.modifyModlist(current_entry[0], attr_dict, ignore_oldexistent=1)
+        ldif = modifyModlist_finegrained(current_entry[0], attr_dict)
 
-        if ldif == []:
+        if not ldif:
             logger.debug("Nothing to update in LDAP")
             return True
 
@@ -273,19 +330,23 @@ def update(self, rdn, attr_dict, new_rdn=False):
                 new_base = dn.split(",", 1)[1]
                 dn = new_rdn + "," + new_base
 
-            for i, (a, k, vs) in enumerate(ldif):
-                if isinstance(vs, list):
-                    vs = [v.encode("utf-8") for v in vs]
-                elif isinstance(vs, str):
-                    vs = [vs.encode("utf-8")]
-                ldif[i] = (a, k, vs)
+            # mod_op : 0 ADD, 1 DELETE, 2 REPLACE
+            for i, (mod_op, attribute, values) in enumerate(ldif):
+                if isinstance(values, list):
+                    values = [v.encode("utf-8") for v in values]
+                elif isinstance(values, str):
+                    values = [values.encode("utf-8")]
+                ldif[i] = (mod_op, attribute, values)
 
             self.con.modify_ext_s(dn, ldif)
         except Exception as e:
             raise MoulinetteError(
-                "error during LDAP update operation with: rdn='%s', "
-                "attr_dict=%s, new_rdn=%s and exception: %s"
-                % (rdn, attr_dict, new_rdn, e),
+                "Error during LDAP update operation:\n"
+                f" rdn: {rdn}\n"
+                f" attr_dict: {attr_dict}\n"
+                f" new_rdn: {new_rdn}\n"
+                f" ldif: {ldif}\n"
+                f" exception: {e}",
                 raw_msg=True,
             )
         else: