From f4d165b157adc5956725c05e338ca04639444f79 Mon Sep 17 00:00:00 2001 From: ljf Date: Mon, 21 Oct 2024 18:35:54 +0200 Subject: [PATCH 1/5] [fix] Avoid 2 sync_perm in admin user creation --- src/user.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/user.py b/src/user.py index 606e8abd54..b67f56d579 100644 --- a/src/user.py +++ b/src/user.py @@ -293,9 +293,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 = { From 0de107ee49d12e62c8716c16985c73ac2ac8cb5a Mon Sep 17 00:00:00 2001 From: ljf Date: Mon, 21 Oct 2024 18:36:21 +0200 Subject: [PATCH 2/5] [enh] Improve perf of ldap.update --- src/utils/ldap.py | 47 +++++++++++++++++++++++++++++++++++++++-------- 1 file changed, 39 insertions(+), 8 deletions(-) diff --git a/src/utils/ldap.py b/src/utils/ldap.py index 98ab356cc7..aaeade774e 100644 --- a/src/utils/ldap.py +++ b/src/utils/ldap.py @@ -242,8 +242,38 @@ def update(self, rdn, attr_dict, new_rdn=False): """ dn = rdn + "," + self.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 = [] + for attribute, value in attr_dict.items(): + if attribute in current_entry[0]: + current_value = current_entry[0][attribute] + if value == current_value: + continue + + # Add or/and delete only needed values in set or list + if isinstance(value, (set, list)) and isinstance(current_value, (set, list)): + values_to_add = list(set(value) - set(current_value)) + if values_to_add: + ldif.append((ldap.MOD_ADD, attribute, values_to_add)) + values_to_del = list(set(current_value) - set(value)) + if values_to_del: + ldif.append((ldap.MOD_DELETE, attribute, values_to_del)) + else: + # Use MOD_REPLACE instead of MOD_DELETE and next MOD_ADD + if isinstance(value, set): + value = list(value) + ldif.append((ldap.MOD_REPLACE, attribute, value)) + else: + if isinstance(value, set): + value = list(value) + ldif.append((ldap.MOD_ADD, attribute, value)) if ldif == []: logger.debug("Nothing to update in LDAP") @@ -255,12 +285,13 @@ 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: From 4f300a09827c97823e11c8b8a3df1ecc829d624e Mon Sep 17 00:00:00 2001 From: ljf Date: Tue, 22 Oct 2024 04:03:33 +0200 Subject: [PATCH 3/5] [fix] Mail and mail-aliases mixed on update --- src/permission.py | 20 +++++++-------- src/settings.py | 2 +- src/user.py | 58 +++++++++++++++++++------------------------- src/utils/ldap.py | 62 ++++++++++++++++++++++++++++++----------------- 4 files changed, 76 insertions(+), 66 deletions(-) diff --git a/src/permission.py b/src/permission.py index b416b05aba..368762b3e1 100644 --- a/src/permission.py +++ b/src/permission.py @@ -580,10 +580,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: @@ -670,9 +670,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, } @@ -727,15 +727,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: @@ -752,7 +752,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) diff --git a/src/settings.py b/src/settings.py index 4f42183be8..0baafcfd56 100644 --- a/src/settings.py +++ b/src/settings.py @@ -240,7 +240,7 @@ def _apply(self): ldap = _get_ldap_interface() ldap.update( "cn=admins,ou=sudo", - {"sudoOption": ["!authenticate"] if passwordless_sudo else []}, + {"sudoOption": {"!authenticate"} if passwordless_sudo else set()}, ) super()._apply() diff --git a/src/user.py b/src/user.py index b67f56d579..32a6c720a5 100644 --- a/src/user.py +++ b/src/user.py @@ -416,23 +416,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"] = [ + 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"] = [ + 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"] = [ + 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 != "": @@ -451,13 +451,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: @@ -483,7 +484,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}) @@ -512,33 +513,27 @@ 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"]) + 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: @@ -601,7 +596,8 @@ def user_info(username): 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] @@ -1275,14 +1271,10 @@ def user_group_update( logger.info(m18n.n("group_update_aliases", group=groupname)) new_attr_dict["mail"] = 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"]) + {"mailGroup"} + else: + new_attr_dict["objectClass"] = set(group["objectClass"]) - {"mailGroup", "mailAccount"} if new_attr_dict: if not from_import: diff --git a/src/utils/ldap.py b/src/utils/ldap.py index aaeade774e..bf64254c78 100644 --- a/src/utils/ldap.py +++ b/src/utils/ldap.py @@ -243,39 +243,57 @@ def update(self, rdn, attr_dict, new_rdn=False): """ dn = rdn + "," + self.basedn 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 = [] - for attribute, value in attr_dict.items(): - if attribute in current_entry[0]: - current_value = current_entry[0][attribute] - if value == current_value: + + 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 attr_dict.items(): + old_value = old_entry.get(attribute, []) + if value == old_value: continue - # Add or/and delete only needed values in set or list - if isinstance(value, (set, list)) and isinstance(current_value, (set, list)): - values_to_add = list(set(value) - set(current_value)) + # Add or/and delete only needed values with unordered set + if isinstance(value, set) and isinstance(old_value, (set, list)): + values_to_add = list(set(value) - set(old_value)) if values_to_add: ldif.append((ldap.MOD_ADD, attribute, values_to_add)) - values_to_del = list(set(current_value) - set(value)) + values_to_del = list(set(old_value) - set(value)) if values_to_del: ldif.append((ldap.MOD_DELETE, attribute, values_to_del)) + + # Add or/and delete only needed values with ordered list + elif isinstance(value, list) and isinstance(old_value, list): + 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:])) + else: # Use MOD_REPLACE instead of MOD_DELETE and next MOD_ADD if isinstance(value, set): value = list(value) - ldif.append((ldap.MOD_REPLACE, attribute, value)) - else: - if isinstance(value, set): - value = list(value) - ldif.append((ldap.MOD_ADD, attribute, value)) + mod_op = ldap.MOD_ADD if not old_value else ldap.MOD_REPLACE + ldif.append((mod_op, attribute, value)) + return ldif + + # 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 From d12956c884dabc7bd1a7fd9083b6a5fc5e7eb3ec Mon Sep 17 00:00:00 2001 From: ljf Date: Tue, 22 Oct 2024 04:22:31 +0200 Subject: [PATCH 4/5] [enh] Support string in ldap.update entries --- src/permission.py | 12 ++++++------ src/settings.py | 2 +- src/user.py | 28 ++++++++++++++-------------- src/utils/ldap.py | 4 +++- 4 files changed, 24 insertions(+), 22 deletions(-) diff --git a/src/permission.py b/src/permission.py index 368762b3e1..9654d57cdc 100644 --- a/src/permission.py +++ b/src/permission.py @@ -580,10 +580,10 @@ def permission_url( ldap.update( f"cn={permission},ou=permission", { - "URL": {url} if url is not None else set(), + "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: @@ -732,10 +732,10 @@ def _update_ldap_group_permission( } 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: @@ -752,7 +752,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) diff --git a/src/settings.py b/src/settings.py index 0baafcfd56..c07b096299 100644 --- a/src/settings.py +++ b/src/settings.py @@ -240,7 +240,7 @@ def _apply(self): ldap = _get_ldap_interface() ldap.update( "cn=admins,ou=sudo", - {"sudoOption": {"!authenticate"} if passwordless_sudo else set()}, + {"sudoOption": "!authenticate" if passwordless_sudo else set()}, ) super()._apply() diff --git a/src/user.py b/src/user.py index 32a6c720a5..2a5872773f 100644 --- a/src/user.py +++ b/src/user.py @@ -416,23 +416,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 != "": @@ -451,7 +451,7 @@ 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: @@ -527,13 +527,13 @@ def user_update( 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: diff --git a/src/utils/ldap.py b/src/utils/ldap.py index bf64254c78..9b2af6ed75 100644 --- a/src/utils/ldap.py +++ b/src/utils/ldap.py @@ -250,7 +250,9 @@ def modifyModlist_finegrained(old_entry: dict, new_entry: dict) -> list: """ ldif = [] for attribute, value in attr_dict.items(): - old_value = old_entry.get(attribute, []) + if not isinstance(value, (set, list)): + value = {value} + old_value = old_entry.get(attribute, set()) if value == old_value: continue From 62b60561393a341ebf9db52d53a2104ea85d1ef3 Mon Sep 17 00:00:00 2001 From: ljf Date: Tue, 22 Oct 2024 04:58:40 +0200 Subject: [PATCH 5/5] [fix] More MOD_REPLACE --- src/utils/ldap.py | 85 ++++++++++++++++++++++++----------------------- 1 file changed, 44 insertions(+), 41 deletions(-) diff --git a/src/utils/ldap.py b/src/utils/ldap.py index 9b2af6ed75..7166a3744e 100644 --- a/src/utils/ldap.py +++ b/src/utils/ldap.py @@ -68,6 +68,50 @@ 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))) + # 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 + class LDAPInterface: def __init__(self): @@ -244,47 +288,6 @@ def update(self, rdn, attr_dict, new_rdn=False): dn = rdn + "," + self.basedn current_entry = self.search(rdn, attrs=None) - 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 attr_dict.items(): - if not isinstance(value, (set, list)): - value = {value} - old_value = old_entry.get(attribute, set()) - if value == old_value: - continue - - # Add or/and delete only needed values with unordered set - if isinstance(value, set) and isinstance(old_value, (set, list)): - values_to_add = list(set(value) - set(old_value)) - if values_to_add: - ldif.append((ldap.MOD_ADD, attribute, values_to_add)) - values_to_del = list(set(old_value) - set(value)) - if values_to_del: - ldif.append((ldap.MOD_DELETE, attribute, values_to_del)) - - # Add or/and delete only needed values with ordered list - elif isinstance(value, list) and isinstance(old_value, list): - 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:])) - - else: - # Use MOD_REPLACE instead of MOD_DELETE and next MOD_ADD - if isinstance(value, set): - value = list(value) - mod_op = ldap.MOD_ADD if not old_value else ldap.MOD_REPLACE - ldif.append((mod_op, attribute, value)) - return ldif # Previously, we used modifyModlist, which directly uses the lib system libldap # supplied with openldap. Unfortunately, the output of this command was not