Bug 58635 - Implement a generic fix for the correct cleanup() order of simpleLdap objects
Summary: Implement a generic fix for the correct cleanup() order of simpleLdap objects
Status: NEW
Alias: None
Product: UCS
Classification: Unclassified
Component: UDM (Generic)
Version: UCS 5.2
Hardware: Other Linux
: P5 normal
Target Milestone: ---
Assignee: UMC maintainers
QA Contact: UMC maintainers
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2025-09-12 09:49 CEST by Sönke Schwardt-Krummrich
Modified: 2025-09-12 19:53 CEST (History)
1 user (show)

See Also:
What kind of report is it?: ---
What type of bug is this?: ---
Who will be affected by this bug?: ---
How will those affected feel about the bug?: ---
User Pain:
Enterprise Customer affected?:
School Customer affected?:
ISV affected?:
Waiting Support:
Flags outvoted (downgraded) after PO Review:
Ticket number:
Bug group (optional):
Customer ID:
Max CVSS v3 score:

Attachments
Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Sönke Schwardt-Krummrich univentionstaff 2025-09-12 09:49:40 CEST 
Another issue of UDM is, that sometimes users of the Python API perform the cleanup step in the wrong order:
The only correct implementer is the UDM CLI.

AppCenter:
management/univention-appcenter/python/appcenter/udm.py-        obj.remove()
management/univention-appcenter/python/appcenter/udm.py:        udm_objects.performCleanup(obj)

Simple UDM API:
management/univention-directory-manager-modules/modules/univention/udm/modules/generic.py-            self._orig_udm_object.remove(remove_childs=remove_childs)
management/univention-directory-manager-modules/modules/univention/udm/modules/generic.py-        if univention.admin.objects.wantsCleanup(self._orig_udm_object):
management/univention-directory-manager-modules/modules/univention/udm/modules/generic.py:            univention.admin.objects.performCleanup(self._orig_udm_object)

UDM REST API:
management/univention-directory-manager-rest/src/univention/admin/rest/module.py-            obj.remove(remove_childs=recursive)
management/univention-directory-manager-rest/src/univention/admin/rest/module.py-            if cleanup:  # and udm_objects.wantsCleanup(obj)
management/univention-directory-manager-rest/src/univention/admin/rest/module.py:                udm_objects.performCleanup(obj)

UMC UDM Module:
management/univention-management-console-module-udm/umc/python/udm/udm_ldap.py-            obj.remove(remove_childs=recursive)
management/univention-management-console-module-udm/umc/python/udm/udm_ldap.py-            if cleanup:
management/univention-management-console-module-udm/umc/python/udm/udm_ldap.py:                udm_objects.performCleanup(obj)

S4C script:
services/univention-samba4/scripts/purge_s4_computer.py-                obj.remove()
services/univention-samba4/scripts/purge_s4_computer.py-            if univention.admin.objects.wantsCleanup(obj):
services/univention-samba4/scripts/purge_s4_computer.py:                univention.admin.objects.performCleanup(obj)

UCS@school lib:
ucs-school-lib/modules/ucsschool/lib/models/base.py-                udm_obj.remove(remove_childs=True)
ucs-school-lib/modules/ucsschool/lib/models/base.py:                udm_objects.performCleanup(udm_obj)


Additionally, performCleanup doesn't do any error handling but silently throws away every error:
management/univention-directory-manager-modules/modules/univention/admin/objects.py:def performCleanup(object: univention.admin.handlers.simpleLdap) -> None:
management/univention-directory-manager-modules/modules/univention/admin/objects.py-    try:
management/univention-directory-manager-modules/modules/univention/admin/objects.py-        object.cleanup()
management/univention-directory-manager-modules/modules/univention/admin/objects.py-    except Exception:
management/univention-directory-manager-modules/modules/univention/admin/objects.py-        pass  # TODO: add logging

And cleanup() performs a open(), which is a duplicated open() then, which will fail if certain extended attributes are set:

management/univention-directory-manager-modules/modules/univention/admin/handlers/__init__.py:    def cleanup(self) -> None:
management/univention-directory-manager-modules/modules/univention/admin/handlers/__init__.py-        self.open()

Triple open here:
management/univention-directory-manager-modules/modules/univention/admin/handlers/computers/__base.py:    def cleanup(self) -> None:
management/univention-directory-manager-modules/modules/univention/admin/handlers/computers/__base.py-        self.open()
management/univention-directory-manager-modules/modules/univention/admin/handlers/computers/__base.py-        self.nagios_cleanup()
management/univention-directory-manager-modules/modules/univention/admin/handlers/computers/__base.py-        univention.admin.handlers.simpleComputer.cleanup(self)

and here:
management/univention-directory-manager-modules/modules/univention/admin/handlers/computers/ipmanagedclient.py:    def cleanup(self) -> None:
management/univention-directory-manager-modules/modules/univention/admin/handlers/computers/ipmanagedclient.py-        self.open()
management/univention-directory-manager-modules/modules/univention/admin/handlers/computers/ipmanagedclient.py-        self.nagios_cleanup()
management/univention-directory-manager-modules/modules/univention/admin/handlers/computers/ipmanagedclient.py-        univention.admin.handlers.simpleComputer.cleanup(self)

And it is very bad, that the client must perform these steps.
We should, as only computer objects use it, remove these functions and integrate it into the remove() method.

 923     def remove(self, remove_childs: bool = False) -> None:

should be:

 923     def remove(self, remove_childs: bool = False, cleanup: bool = True) -> None:



We should implement a generic solution that fixes it for all modules and remains backwards compatible with the existing code.