Bug 40120 - Diff of attribute values is not order independent
Diff of attribute values is not order independent
Status: CLOSED FIXED
Product: UCS
Classification: Unclassified
Component: UDM (Generic)
UCS 4.1
Other Linux
: P5 normal (vote)
: UCS 4.1-0-errata
Assigned To: Florian Best
Philipp Hahn
:
: 37556 (view as bug list)
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2015-11-27 15:35 CET by Florian Best
Modified: 2016-05-27 10:40 CEST (History)
3 users (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):
Max CVSS v3 score:


Attachments
patch (854 bytes, patch)
2015-11-27 15:35 CET, Florian Best
Details | Diff
40120.py (733 bytes, text/x-python)
2016-01-14 14:46 CET, Florian Best
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Florian Best univentionstaff 2015-11-27 15:35:44 CET
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.
Comment 1 Florian Best univentionstaff 2016-01-07 12:07:14 CET
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
Comment 2 Philipp Hahn univentionstaff 2016-01-13 18:54:44 CET
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'])))"
Comment 3 Florian Best univentionstaff 2016-01-14 14:46:19 CET
Created attachment 7407 [details]
40120.py

A simple python script which shows the problem.
Comment 4 Florian Best univentionstaff 2016-01-14 15:23:29 CET
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?
Comment 5 Philipp Hahn univentionstaff 2016-01-20 13:41:21 CET
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
Comment 6 Janek Walkenhorst univentionstaff 2016-01-27 16:22:49 CET
<http://errata.software-univention.de/ucs/4.1/69.html>
Comment 7 Florian Best univentionstaff 2016-05-27 10:40:47 CEST
*** Bug 37556 has been marked as a duplicate of this bug. ***