Univention Bugzilla – Bug 40120
Diff of attribute values is not order independent
Last modified: 2016-05-27 10:40:47 CEST
Created attachment 7323 [details] patch The order of object classes is not important for ldap. We should compare/diff them as ucs-school-lib.models (+ maybe others, too) is broken and always add a different order to the objects. This causes problems if users don't have the LDAP ACLs to modify the object classes. Attached patch fixes it.
This applies to all attributes not only "objectClass" so a generic fix has been implemented. All modlist items are compared independent of the order of values (if they are lists/tuples and not strings). univention-directory-manager-modules (11.0.2-11): r66618 | Bug #40120: don't depend on order when comparing attributes to create modlist univention-directory-manager-modules.yaml: r66622 | YAML Bug #39993 Bug #40120
OK: r66618 FYI: I just did a code-review, as I have no test-case to trigger the problem mentioned in comment #0. RFC: The change looks incomplete, as the filtering is added in _ldap_modlist(). That function builds the initial modification list (ml) from diffing .oldinfo and .info. That diff is then converted by mapping the UDM property names/values to the LDAP attribute names/values through "mapping.register()"ed. _ldap_modlist() is overwritten by several sub-classes, which re-use this function by calling it through super(). But they add their modification *afterwards*, so the filtering does not get applied for those additional changes. IMHO it would be more useful to add the filtering before calling "self.lo.add()" or "self.lo.modify()" (or therein), which is in univention.admin.uldap, which calls through to univention.uldap. Please note that the add-list (al) and ml is a mix of 2-tuples and 3-tuples, which is only normalized in base/univention-python/modules/uldap.py. Also note that the ml does contain consecutive updates, that is there may be more than one change per LDAP-attribute, so you still get "some values removed to be re-added later on". OK: r66622 OK: univention-directory-manager-modules.yaml OK: errata-announce -V univention-directory-manager-modules.yaml QA: udm computers/memberserver create --set name=test --set password=univention lb=$(ucr get ldap/base) python -c "from univention.admincli.admin import doit;print('\n'.join(doit(['','computers/memberserver','modify','--binddn','cn=test,$lb','--bindpwd','univention','--dn','cn=test,$lb','--set','operatingSystem=$RANDOM'])))"
Created attachment 7407 [details] 40120.py A simple python script which shows the problem.
univention-python (9.0.1-2): r66797 | Bug #40120: don't depend on attribute order when building modlist (In reply to Philipp Hahn from comment #2) > OK: r66618 > FYI: I just did a code-review, as I have no test-case to trigger the problem > mentioned in comment #0. → attachment 7407 [details] > RFC: The change looks incomplete, as the filtering is added in > _ldap_modlist(). > That function builds the initial modification list (ml) from diffing > .oldinfo and .info. > That diff is then converted by mapping the UDM property names/values to the > LDAP attribute names/values through "mapping.register()"ed. yes. > _ldap_modlist() is overwritten by several sub-classes, which re-use this > function by calling it through super(). But they add their modification > *afterwards*, so the filtering does not get applied for those additional > changes. yes. > IMHO it would be more useful to add the filtering before calling > "self.lo.add()" or "self.lo.modify()" (or therein), which is in > univention.admin.uldap, which calls through to univention.uldap. > Please note that the add-list (al) and ml is a mix of 2-tuples and > 3-tuples, which is only normalized in > base/univention-python/modules/uldap.py. Yes, therefore access.modify() has been adjusted to make the final diff. The other change has been reverted. The modlist in modify() is always a 3-tuple. There is no need to adjust addlist because there is no comparision!?! > Also note that the ml does contain consecutive updates, that is there may > be more than one change per LDAP-attribute, so you still get "some values > removed to be re-added later on". ok, this is okay with the new implementation. > QA: > udm computers/memberserver create --set name=test --set password=univention > lb=$(ucr get ldap/base) > python -c "from univention.admincli.admin import > doit;print('\n'.join(doit(['','computers/memberserver','modify','--binddn', > 'cn=test,$lb','--bindpwd','univention','--dn','cn=test,$lb','--set', > 'operatingSystem=$RANDOM'])))" → This is to check if string comparison is not affected?
OK: errata-announce -BBV univention-python.yaml OK: univention-python.yaml OK: univention-directory-manager-modules.yaml OK: univention-directory-manager-modules reverted OK: cleanup/spelling fix remains, CodeReview OK: univention-python CodeReview OK: ucs-test detected no error r66905 | Bug #40120 test/LDAP: Ignore order of multi-valued attributes on modify added 10_ldap/84acl_multivalued_ordering Package: ucs-test Version: 6.0.31-16.1389.201601201339 Branch: ucs_4.1-0 Scope: errata4.1-0
<http://errata.software-univention.de/ucs/4.1/69.html>
*** Bug 37556 has been marked as a duplicate of this bug. ***