From aed1e63eab4d02662ff2923401a6ef0c1d72b6b1 Mon Sep 17 00:00:00 2001 From: Lukas Oyen Date: Thu, 17 Aug 2017 17:51:35 +0200 Subject: [PATCH 1/4] Bug #45252: s4c: fix attribute deduplication: preserve order --- .../modules/univention/s4connector/__init__.py | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/services/univention-s4-connector/modules/univention/s4connector/__init__.py b/services/univention-s4-connector/modules/univention/s4connector/__init__.py index f22c5e5..9f6791a 100644 --- a/services/univention-s4-connector/modules/univention/s4connector/__init__.py +++ b/services/univention-s4-connector/modules/univention/s4connector/__init__.py @@ -43,6 +43,7 @@ import traceback import types import ldap import univention.uldap +from univention.lib import ordered_set import univention.admin.uldap import univention.admin.modules import univention.admin.objects @@ -1207,13 +1208,11 @@ class ucs: else: equal = compare[0] == compare[1] if not equal: - # This is deduplication of LDAP attribute values for S4 -> UCS. - # It destroys ordering of multi-valued attributes. This seems problematic - # as the handling of `con_other_attribute` assumes preserved ordering - # (this is not guaranteed by LDAP). + # This is deduplication of LDAP attribute values + # for S4 -> UCS with preserved order. # See the MODIFY-case in `sync_from_ucs()` for more. if isinstance(value, list): - ucs_object[ucs_key] = list(set(value)) + ucs_object[ucs_key] = list(ordered_set.OrderedSet(value)) else: ucs_object[ucs_key] = value ud.debug(ud.LDAP, ud.INFO, "set key in ucs-object: %s" % ucs_key) -- 2.7.4 From df9e6edeff258a9b467f9a207133063287ce25c5 Mon Sep 17 00:00:00 2001 From: Lukas Oyen Date: Thu, 17 Aug 2017 18:21:55 +0200 Subject: [PATCH 2/4] Bug #45252: s4c: cleanup `sync_from_ucs()` MODIFY case --- .../modules/univention/s4connector/s4/__init__.py | 253 +++++++++------------ 1 file changed, 113 insertions(+), 140 deletions(-) diff --git a/services/univention-s4-connector/modules/univention/s4connector/s4/__init__.py b/services/univention-s4-connector/modules/univention/s4connector/s4/__init__.py index c7b9ffb..b30a2df 100644 --- a/services/univention-s4-connector/modules/univention/s4connector/s4/__init__.py +++ b/services/univention-s4-connector/modules/univention/s4connector/s4/__init__.py @@ -2579,160 +2579,133 @@ class s4(univention.s4connector.ucs): # elif (object['modtype'] == 'modify' and s4_object) or (object['modtype'] == 'add' and s4_object): ud.debug(ud.LDAP, ud.INFO, "sync_from_ucs: modify object: %s" % object['dn']) - ud.debug(ud.LDAP, ud.INFO, "sync_from_ucs: old_object: %s" % old_ucs_object) - ud.debug(ud.LDAP, ud.INFO, "sync_from_ucs: new_object: %s" % new_ucs_object) + ud.debug(ud.LDAP, ud.INFO, + "sync_from_ucs: old_object: %s" % old_ucs_object) + ud.debug(ud.LDAP, ud.INFO, + "sync_from_ucs: new_object: %s" % new_ucs_object) object['old_ucs_object'] = old_ucs_object object['new_ucs_object'] = new_ucs_object - attribute_list = set(old_ucs_object.keys()).union(set(new_ucs_object.keys())) - if hasattr(self.property[property_type], "con_sync_function"): - self.property[property_type].con_sync_function(self, property_type, object) - else: - # Iterate over attributes and post_attributes - for attribute_type_name, attribute_type in [('attributes', self.property[property_type].attributes), ('post_attributes', self.property[property_type].post_attributes)]: - if hasattr(self.property[property_type], attribute_type_name) and attribute_type is not None: - for attr in attribute_list: - value = new_ucs_object.get(attr) - if not self.__has_attribute_value_changed(attr, old_ucs_object, new_ucs_object): - continue + attribute_list = set(old_ucs_object.keys() + new_ucs_object.keys()) - ud.debug(ud.LDAP, ud.INFO, "sync_from_ucs: The following attribute has been changed: %s" % attr) - - for attribute in attribute_type.keys(): - if attribute_type[attribute].ldap_attribute != attr: - continue + def find_case_independent(s4_object, attribute): + attr = attribute.lower() + matching = (v for (k, v) in s4_object.iteritems() if k.lower() == attr) + try: + values = next(matching) + except StopIteration: + values = [] + return set(values) + + # Iterate over attributes and post_attributes + for attribute_type_name, attribute_type in [('attributes', self.property[property_type].attributes), + ('post_attributes', self.property[property_type].post_attributes)]: + if hasattr(self.property[property_type], attribute_type_name) and attribute_type is not None: + for attr in attribute_list: + if not self.__has_attribute_value_changed(attr, old_ucs_object, new_ucs_object): + continue - ud.debug(ud.LDAP, ud.INFO, "sync_from_ucs: Found a corresponding mapping defintion: %s" % attribute) - s4_attribute = attribute_type[attribute].con_attribute - s4_other_attribute = attribute_type[attribute].con_other_attribute + ud.debug(ud.LDAP, ud.INFO, "sync_from_ucs: The following attribute has been changed: %s" % attr) - if not attribute_type[attribute].sync_mode in ['write', 'sync']: - ud.debug(ud.LDAP, ud.INFO, "sync_from_ucs: %s is in not in write or sync mode. Skipping" % attribute) - continue + for attribute in attribute_type.keys(): + if attribute_type[attribute].ldap_attribute != attr: + continue - modify = False + ud.debug(ud.LDAP, ud.INFO, "sync_from_ucs: Found a corresponding mapping defintion: %s" % attribute) + s4_attribute = attribute_type[attribute].con_attribute + s4_other_attribute = attribute_type[attribute].con_other_attribute - # Get the UCS attributes - old_values = set(old_ucs_object.get(attr, [])) - new_values = set(new_ucs_object.get(attr, [])) + if not attribute_type[attribute].sync_mode in ['write', 'sync']: + ud.debug(ud.LDAP, ud.INFO, "sync_from_ucs: %s is in not in write or sync mode. Skipping" % attribute) + continue - ud.debug(ud.LDAP, ud.INFO, "sync_from_ucs: old_values: %s" % old_values) - ud.debug(ud.LDAP, ud.INFO, "sync_from_ucs: new_values: %s" % new_values) + # Get the UCS attributes + old_values = set(old_ucs_object.get(attr, [])) + new_values = set(new_ucs_object.get(attr, [])) - if attribute_type[attribute].compare_function: - if not attribute_type[attribute].compare_function(list(old_values), list(new_values)): - modify = True - elif not univention.s4connector.compare_lowercase(list(old_values), list(new_values)): # FIXME: use defined compare-function from mapping.py - modify = True + ud.debug(ud.LDAP, ud.INFO, "sync_from_ucs: old_values: %s" % old_values) + ud.debug(ud.LDAP, ud.INFO, "sync_from_ucs: new_values: %s" % new_values) - if not modify: - ud.debug(ud.LDAP, ud.INFO, "sync_from_ucs: no modification necessary for %s" % attribute) - continue + current_s4_values = find_case_independent(s4_object, s4_attribute) - # So, at this point we have the old and the new UCS object. - # Thus we can create the diff, but we have to check the current S4 object + compare_function = attribute_type[attribute].compare_function or \ + univention.s4connector.compare_lowercase + if compare_function(list(old_values), list(new_values)): + ud.debug(ud.LDAP, ud.INFO, "sync_from_ucs: no modification necessary for %s" % attribute) + continue - if not old_values: - to_add = new_values - to_remove = set([]) - elif not new_values: - to_remove = old_values - to_add = set([]) - else: - to_add = new_values - old_values - to_remove = old_values - new_values - - if s4_other_attribute: - # This is the case, where we map from a multi-valued UCS attribute to two S4 attributes. - # telephoneNumber/otherTelephone (S4) to telephoneNumber (UCS) would be an example. - # - # The direct mapping assumes preserved ordering of the multi-valued UCS - # attributes and places the first value in the primary S4 attribute, - # the rest in the secondary S4 attributes. - # Assuming preserved ordering is wrong, as LDAP does not guarantee is and the - # deduplication of LDAP attribute values in `__set_values()` destroys it. - # - # The following code handles the correct distribution of the UCS attribute, - # to two S4 attributes. It also ensures, that the primary S4 attribute keeps - # its value as long as that value is not removed. If removed the primary - # attribute is assigned a random value from the UCS attribute. - try: - current_s4_values = set([v for k, v in s4_object.iteritems() if s4_attribute.lower() == k.lower()][0]) - except IndexError: - current_s4_values = set([]) - ud.debug(ud.LDAP, ud.INFO, "sync_from_ucs: The current S4 values: %s" % current_s4_values) - - try: - current_s4_other_values = set([v for k, v in s4_object.iteritems() if s4_other_attribute.lower() == k.lower()][0]) - except IndexError: - current_s4_other_values = set([]) - ud.debug(ud.LDAP, ud.INFO, "sync_from_ucs: The current S4 other values: %s" % current_s4_other_values) - - new_s4_values = current_s4_values - to_remove - if not new_s4_values and to_add: - for n_value in new_ucs_object.get(attr, []): - if n_value in to_add: - to_add = to_add - set([n_value]) - new_s4_values = [n_value] - break - - new_s4_other_values = (current_s4_other_values | to_add) - to_remove - current_s4_values - if current_s4_values != new_s4_values: - if new_s4_values: - modlist.append((ldap.MOD_REPLACE, s4_attribute, new_s4_values)) - else: - modlist.append((ldap.MOD_REPLACE, s4_attribute, [])) - - if current_s4_other_values != new_s4_other_values: - modlist.append((ldap.MOD_REPLACE, s4_other_attribute, new_s4_other_values)) + # So, at this point we have the old and the new UCS object. + # Thus we can create the diff, but we have to check the current S4 object + to_add = new_values - old_values + to_remove = old_values - new_values + + if s4_other_attribute: + # This is the case, where we map from a multi-valued UCS attribute to two S4 attributes. + # telephoneNumber/otherTelephone (S4) to telephoneNumber (UCS) would be an example. + # + # The direct mapping assumes preserved ordering of the multi-valued UCS + # attributes and places the first value in the primary S4 attribute, + # the rest in the secondary S4 attributes. + # + # The following code handles the correct distribution of the UCS attribute, + # to two S4 attributes. It also ensures, that the primary S4 attribute keeps + # its value as long as that value is not removed. If removed the primary + # attribute is assigned a random value from the UCS attribute. + current_s4_other_values = find_case_independent(s4_object, s4_other_attribute) + + ud.debug(ud.LDAP, ud.INFO, "sync_from_ucs: The current S4 values: %s" % current_s4_values) + ud.debug(ud.LDAP, ud.INFO, "sync_from_ucs: The current S4 other values: %s" % current_s4_other_values) + + # If we removed the value on the UCS side that was contained in the `s4_attribute`, + # but are adding new values, we choose a random value from the new values. + new_s4_values = current_s4_values - to_remove + if not new_s4_values and to_add: + new_s4_values.add(to_add.pop()) + new_s4_other_values = (current_s4_other_values | to_add) - to_remove - current_s4_values + + if current_s4_values != new_s4_values: + modlist.append((ldap.MOD_REPLACE, s4_attribute, new_s4_values)) + if current_s4_other_values != new_s4_other_values: + modlist.append((ldap.MOD_REPLACE, s4_other_attribute, new_s4_other_values)) + else: + ud.debug(ud.LDAP, ud.INFO, "sync_from_ucs: The current S4 values: %s" % current_s4_values) + + if (to_add or to_remove) and attribute_type[attribute].single_value: + value = new_ucs_object.get(attr) + modify = not current_s4_values or not value or \ + not compare_function(list(current_s4_values), list(value)) + if modify: + mapping = getattr(attribute_type[attribute], 'mapping', ()) + if len(mapping) > 0 and mapping[0]: + ud.debug(ud.LDAP, ud.PROCESS, "Calling single value mapping function") + value = mapping[0](self, None, object) + modlist.append((ldap.MOD_REPLACE, s4_attribute, value)) else: - try: - current_s4_values = set([v for k, v in s4_object.iteritems() if s4_attribute.lower() == k.lower()][0]) - except IndexError: - current_s4_values = set([]) - - ud.debug(ud.LDAP, ud.INFO, "sync_from_ucs: The current S4 values: %s" % current_s4_values) - - if (to_add or to_remove) and attribute_type[attribute].single_value: - modify = False - if not current_s4_values or not value: - modify = True - elif attribute_type[attribute].compare_function: - if not attribute_type[attribute].compare_function(list(current_s4_values), list(value)): - modify = True - elif not univention.s4connector.compare_lowercase(list(current_s4_values), list(value)): - modify = True - if modify: - if hasattr(attribute_type[attribute], 'mapping') and len(attribute_type[attribute].mapping) > 0 and attribute_type[attribute].mapping[0]: - ud.debug(ud.LDAP, ud.PROCESS, "Calling single value mapping function") - value = attribute_type[attribute].mapping[0](self, None, object) - modlist.append((ldap.MOD_REPLACE, s4_attribute, value)) - else: - if to_remove: - r = current_s4_values & to_remove - if r: - modlist.append((ldap.MOD_DELETE, s4_attribute, r)) - if to_add: - a = to_add - current_s4_values - if a: - modlist.append((ldap.MOD_ADD, s4_attribute, a)) - - if not modlist: - ud.debug(ud.LDAP, ud.ALL, "nothing to modify: %s" % object['dn']) - else: - ud.debug(ud.LDAP, ud.INFO, "to modify: %s" % object['dn']) - ud.debug(ud.LDAP, ud.ALL, "sync_from_ucs: modlist: %s" % modlist) - try: - self.lo_s4.lo.modify_ext_s(compatible_modstring(object['dn']), compatible_modlist(modlist), serverctrls=self.serverctrls_for_add_and_modify) - except: - ud.debug(ud.LDAP, ud.ERROR, "sync_from_ucs: traceback during modify object: %s" % object['dn']) - ud.debug(ud.LDAP, ud.ERROR, "sync_from_ucs: traceback due to modlist: %s" % modlist) - raise + if to_remove: + r = current_s4_values & to_remove + if r: + modlist.append((ldap.MOD_DELETE, s4_attribute, r)) + if to_add: + a = to_add - current_s4_values + if a: + modlist.append((ldap.MOD_ADD, s4_attribute, a)) + + if not modlist: + ud.debug(ud.LDAP, ud.ALL, "nothing to modify: %s" % object['dn']) + else: + ud.debug(ud.LDAP, ud.INFO, "to modify: %s" % object['dn']) + ud.debug(ud.LDAP, ud.ALL, "sync_from_ucs: modlist: %s" % modlist) + try: + self.lo_s4.lo.modify_ext_s(compatible_modstring(object['dn']), compatible_modlist(modlist), serverctrls=self.serverctrls_for_add_and_modify) + except: + ud.debug(ud.LDAP, ud.ERROR, "sync_from_ucs: traceback during modify object: %s" % object['dn']) + ud.debug(ud.LDAP, ud.ERROR, "sync_from_ucs: traceback due to modlist: %s" % modlist) + raise - if hasattr(self.property[property_type], "post_con_modify_functions"): - for f in self.property[property_type].post_con_modify_functions: - ud.debug(ud.LDAP, ud.INFO, "Call post_con_modify_functions: %s" % f) - f(self, property_type, object) - ud.debug(ud.LDAP, ud.INFO, "Call post_con_modify_functions: %s (done)" % f) + if hasattr(self.property[property_type], "post_con_modify_functions"): + for f in self.property[property_type].post_con_modify_functions: + ud.debug(ud.LDAP, ud.INFO, "Call post_con_modify_functions: %s" % f) + f(self, property_type, object) + ud.debug(ud.LDAP, ud.INFO, "Call post_con_modify_functions: %s (done)" % f) # # DELETE # -- 2.7.4 From d9b4823072d31179fa43bd543000c5cb26d6f202 Mon Sep 17 00:00:00 2001 From: Lukas Oyen Date: Mon, 21 Aug 2017 11:01:03 +0200 Subject: [PATCH 3/4] Bug #45252: s4c: implement correct `con_other_attribute` handling --- .../modules/univention/s4connector/s4/__init__.py | 57 ++++++++++++++++------ 1 file changed, 41 insertions(+), 16 deletions(-) diff --git a/services/univention-s4-connector/modules/univention/s4connector/s4/__init__.py b/services/univention-s4-connector/modules/univention/s4connector/s4/__init__.py index b30a2df..3de6e52 100644 --- a/services/univention-s4-connector/modules/univention/s4connector/s4/__init__.py +++ b/services/univention-s4-connector/modules/univention/s4connector/s4/__init__.py @@ -42,6 +42,7 @@ import time import types import array import univention.uldap +from univention.lib import ordered_set import univention.s4connector import univention.debug2 as ud from ldap.controls import LDAPControl @@ -51,6 +52,7 @@ from samba.dcerpc import security from samba.ndr import ndr_pack, ndr_unpack from samba.dcerpc import misc + DECODE_IGNORELIST = ['objectSid', 'objectGUID', 'repsFrom', 'replUpToDateVector', 'ipsecData', 'logonHours', 'userCertificate', 'dNSProperty', 'dnsRecord'] LDAP_SERVER_SHOW_DELETED_OID = "1.2.840.113556.1.4.417" @@ -2585,7 +2587,7 @@ class s4(univention.s4connector.ucs): "sync_from_ucs: new_object: %s" % new_ucs_object) object['old_ucs_object'] = old_ucs_object object['new_ucs_object'] = new_ucs_object - attribute_list = set(old_ucs_object.keys() + new_ucs_object.keys()) + attribute_list = ordered_set.OrderedSet(old_ucs_object.keys() + new_ucs_object.keys()) def find_case_independent(s4_object, attribute): attr = attribute.lower() @@ -2594,7 +2596,7 @@ class s4(univention.s4connector.ucs): values = next(matching) except StopIteration: values = [] - return set(values) + return ordered_set.OrderedSet(values) # Iterate over attributes and post_attributes for attribute_type_name, attribute_type in [('attributes', self.property[property_type].attributes), @@ -2619,8 +2621,8 @@ class s4(univention.s4connector.ucs): continue # Get the UCS attributes - old_values = set(old_ucs_object.get(attr, [])) - new_values = set(new_ucs_object.get(attr, [])) + old_values = ordered_set.OrderedSet(old_ucs_object.get(attr, [])) + new_values = ordered_set.OrderedSet(new_ucs_object.get(attr, [])) ud.debug(ud.LDAP, ud.INFO, "sync_from_ucs: old_values: %s" % old_values) ud.debug(ud.LDAP, ud.INFO, "sync_from_ucs: new_values: %s" % new_values) @@ -2638,29 +2640,52 @@ class s4(univention.s4connector.ucs): to_add = new_values - old_values to_remove = old_values - new_values + ud.debug(ud.LDAP, ud.INFO, "sync_from_ucs: to_add: %s" % to_add) + ud.debug(ud.LDAP, ud.INFO, "sync_from_ucs: to_remove: %s" % to_remove) + if s4_other_attribute: # This is the case, where we map from a multi-valued UCS attribute to two S4 attributes. # telephoneNumber/otherTelephone (S4) to telephoneNumber (UCS) would be an example. # - # The direct mapping assumes preserved ordering of the multi-valued UCS - # attributes and places the first value in the primary S4 attribute, - # the rest in the secondary S4 attributes. + # In Active Directory, for attributes that are split in two the administrator is + # responsible for keeping a value in `telephoneNumber`. Imagine the following: + # (a) telephoneNumber = '123', otherTelephone = ['123', '456'] + # In this case, if the administrator deletes the value of `telephoneNumber`, + # Active Directory does NOT automatically pull a new value from `otherTelephone`. + # + # This is impossible to support with the connector. Imagine again case (a). If + # we delete `123` from `phone` via UDM, AD would be synced into the following + # state: (b) telephoneNumber = '', otherTelephone = ['456'] + # From now on, whenever we add a new value to `phone` via UDM, for example: + # (c) phone = ['456', '789'] it MUST be synced as + # (d) telephoneNumber = '', otherTelephone = ['456', '789'] as '456' came + # before '789' and '456' is definitely in `otherTelephone`. # - # The following code handles the correct distribution of the UCS attribute, - # to two S4 attributes. It also ensures, that the primary S4 attribute keeps - # its value as long as that value is not removed. If removed the primary - # attribute is assigned a random value from the UCS attribute. + # We therefore implement, that `telephoneNumber` is never empty, as long as there + # are values in `otherTelephone`. If a modification would delete the value of + # `telephoneNumber` and at least one value exists in `otherTelephone`, the + # connector duplicates the first entry of `otherTelephone` into + # `telephoneNumber`. current_s4_other_values = find_case_independent(s4_object, s4_other_attribute) ud.debug(ud.LDAP, ud.INFO, "sync_from_ucs: The current S4 values: %s" % current_s4_values) ud.debug(ud.LDAP, ud.INFO, "sync_from_ucs: The current S4 other values: %s" % current_s4_other_values) - # If we removed the value on the UCS side that was contained in the `s4_attribute`, - # but are adding new values, we choose a random value from the new values. new_s4_values = current_s4_values - to_remove - if not new_s4_values and to_add: - new_s4_values.add(to_add.pop()) - new_s4_other_values = (current_s4_other_values | to_add) - to_remove - current_s4_values + retained_s4_other_values = current_s4_other_values - to_remove + + # If we removed the value on the UCS side that was contained in the `current_s4_values`, + # but have values, we duplicate the first value from the `new_s4_other_values`. + if not new_s4_values: + if retained_s4_other_values: + new_s4_values.add(retained_s4_other_values[0]) + elif to_add: + new_s4_values.add(to_add[0]) + + # Take the old values without those to be removed. Add the new ones + # (without duplicating the `new_s4_values`, but preserving existing + # duplicates in `con_other_attribute`) + new_s4_other_values = retained_s4_other_values | (to_add - new_s4_values) if current_s4_values != new_s4_values: modlist.append((ldap.MOD_REPLACE, s4_attribute, new_s4_values)) -- 2.7.4 From 92e05137b6cc170fa1a3aba67a89f805e69ba1a9 Mon Sep 17 00:00:00 2001 From: Lukas Oyen Date: Tue, 5 Sep 2017 15:33:25 +0200 Subject: [PATCH 4/4] Revert "Bug #45252: s4c-test: skip `con_other_attribute` sync test" --- test/ucs-test/tests/52_s4connector/502_other_attribute_sync.py | 5 ----- 1 file changed, 5 deletions(-) diff --git a/test/ucs-test/tests/52_s4connector/502_other_attribute_sync.py b/test/ucs-test/tests/52_s4connector/502_other_attribute_sync.py index 26efa7b..5cb9b23 100755 --- a/test/ucs-test/tests/52_s4connector/502_other_attribute_sync.py +++ b/test/ucs-test/tests/52_s4connector/502_other_attribute_sync.py @@ -8,11 +8,6 @@ ## - 35903 ## - 36480 ## - 45252 -## versions: -## 4.2-2: skip - -# We skip this since 4.2-2, as the corresponding implementation is not yet committed. -# See https://forge.univention.org/bugzilla/show_bug.cgi?id=45252. import pytest -- 2.7.4